Bug 1721720

Phil,
I roughly went through the commits, mostly they're cli fixes, additional tests or sanity patches.

276c452e47c5e ("netlink: remove markup json parsing code")
says: We have better json support these days, remove libnftnl json support
 Is this API change of the library that could impact some of it's consumers? There are few functions removed. If so, can we postpone this change for next release?

Also, could you please provide general suggestions for areas to test? Some commits include testsuite updates, but not all of them.

Thanks,
Tomas

Hi Tomas,

(In reply to Tomas Dolezal from comment #1)
> I roughly went through the commits, mostly they're cli fixes, additional
> tests or sanity patches.

Thanks for the review!

> 276c452e47c5e ("netlink: remove markup json parsing code")
> says: We have better json support these days, remove libnftnl json support
>  Is this API change of the library that could impact some of it's consumers?
> There are few functions removed. If so, can we postpone this change for next
> release?

It is not a library change, the commit message is a bit misleading: In fact,
this commit removes nft's support for the JSON import functionality in
libnftnl. So practically 'nft import vm json' does no longer work. But we don't
ship libnftnl with JSON parsing support, so the only effect is that this
command fails earlier (namely, before calling libnftnl functions instead of
after).

The real reason why I've included it is the removal of the failing
vm_json_import_0 testcase.

> Also, could you please provide general suggestions for areas to test? Some
> commits include testsuite updates, but not all of them.

I'd say the usual CI testing is sufficient. Does it include nftables testsuites
already? OTOH I realize that CI didn't catch the issues fixed by these
backports in the first place, so it can't be sufficient for this ticket. I'll
come up with a list of tests/reproducers for the bugs fixed in the proposed
series next week. :)

Thanks, Phil

Testing instructions for each of the commits to backport:


* bbbed9f3175c5 ("datatype: add stolen verdict")

ip netns add test
ip link add v0 type veth peer name v0 netns test
ip link set v0 up
ip link add d0 type dummy
ip link set d0 up
nft -f - <<EOF
table netdev t {
	chain c {
		type filter hook ingress device "v0" priority 0

		meta nftrace set 1
		fwd to d0
	}
}
nft monitor trace &
ip -netns test link set v0 up
ip -netns test a a 10.0.0.1/24 dev v0
ip netns exec test ping 10.0.0.2


* 5ca7ad2523668 ("libnftables: Fix exit_cookie()")

requires C code to test:

nft = nft_ctx_new(0);
nft_ctx_buffer_output(nft);
nft_ctx_unbuffer_output(nft);
nft_ctx_buffer_output(nft);

Sanity only?


* b652c8a14c3cd ("scanner: Do not convert tabs into spaces")

nft -f - <<EOF
		add chain ip nonexistent c
EOF
		add chain ip nonexistent c
		                           ^^^^^^^^^^^

(Note there are two tabs at beginning of input line.) Markers should be aligned
with table name.


* 056aaa3e6dc65 ("netlink_delinearize: Refactor meta_may_dependency_kill()")

Make sure tests/py/inet/icmp.t passes.


* 6b00b9537e181 ("evaluate: skip evaluation of datatype concatenations")

(see https://bugzilla.netfilter.org/show_bug.cgi?id=1265)

nft create set inet filter keepalived_ranges4 { type inet_service . ifname \; }
Error: Empty string is not allowed


* bf91cfd9a6194 ("tests: shell: add tests for listing objects")

Dependency for next one, just some new tests.


* dafac7d528de0 ("rule: fix object listing when no table is given")

Make sure tests/shell/testcases/listing/0014objects_0 passes.


* 0f44d4f627535 ("proto: fix icmp/icmpv6 code datatype")

Make sure tests/py/ip/icmp.t and tests/py/ip6/icmpv6.t pass.


* 5b35fb3132b1f ("evaluate: throw distinct error if map exists but contains no objects")

nft -f - <<EOF
table ip filter {
	map foo {
		type inet_service : ifname
	}
	chain in {
	}
}
EOF
nft add rule filter in ct helper set tcp dport map @foo
Error: Expression is not a map
add rule filter in ct helper set tcp dport map @foo
                                               ^^^^

Error message should be: "Expression is not a map with objects"


* 1018eae77176c ("parser: bail out on incorrect burst unit")

Make sure tests/py/any/limit.t passes.


* b338244abc7f0 ("src: fix netdev family device name parsing")

Chain in netdev family (see above) must be listed with device name in quotes,
also quoted device name must be allowed.


* a0da4c5bbf0d7 ("libnftables: Print errors before freeing commands")

Not sure how to reproduce a crash due to that bug. Sanity only?


* afd1ad6f68680 ("segtree: fix crash when debug mode is active")

nft --debug=segtree add rule ip t c tcp dport { 22-33 } accept
insert: [16 21]
iter: [16 21]
list: [0000 0015]
list: [0016 0021]
list: [0022 ffff]
Segmentation fault (core dumped)


* d3cace2660925 ("parser_bison: no need for statement separator for ct object commands")

# nft add table ip t
# nft add ct helper ip t cth '{ type "ftp" protocol tcp; }; add chain ip t c'
Error: syntax error, unexpected add, expecting end of file or newline or semicolon
add ct helper ip t cth { type "ftp" protocol tcp; }; add chain ip t c
                                                     ^^^


* e07be57df3f51 ("ct: use nft_print() instead of printf()")

Print flow offload statement after changing output_fp to something else. Sanity only?


* 4d97f0a4eebd2 ("parser_bison: type_identifier string memleak")
* 4ac11b890fe87 ("src: missing destroy function in statement definitions")

Memleak fixes, sanity only?


* 760a8bab07ade ("tests: shell: validate too deep jumpstack from basechain")

Make sure tests/shell/testcases/chains/0002jumps_1 passes.


* 276c452e47c5e ("netlink: remove markup json parsing code")

Sanity only?


* 1dc9be8445265 ("rule: limit: don't print default burst value")

Make sure tests/shell/testcases/sets/0026named_limit_0 passes.

CI testing exposed an outstanding bug in the above series, to fix it I had to backport three further commits (the first two are dependencies):

c15c2869168d7 ("xt: pass octx to translate function")
b3c8de9c5aecd ("xt: always build with a minimal support for xt match/target decode")
99afd62d48f4c ("src: fix double free on xt stmt destruction")

Existing CI tests for xtables support is sufficient in testing these.

*** Bug 1682994 has been marked as a duplicate of this bug. ***

(In reply to Phil Sutter from comment #5)
> Testing instructions for each of the commits to backport:
Thanks for providing steps for automated coverage!
 
> * bbbed9f3175c5 ("datatype: add stolen verdict")
automated

> * 5ca7ad2523668 ("libnftables: Fix exit_cookie()")
> requires C code to test:
> Sanity only?
sanity, not covered

> * b652c8a14c3cd ("scanner: Do not convert tabs into spaces")
automated

> * 056aaa3e6dc65 ("netlink_delinearize: Refactor meta_may_dependency_kill()")
> Make sure tests/py/inet/icmp.t passes.
unable to execute tests/py/ because of broken deps. integration bug 1754047

> * 6b00b9537e181 ("evaluate: skip evaluation of datatype concatenations")
automated

> * bf91cfd9a6194 ("tests: shell: add tests for listing objects")
> * dafac7d528de0 ("rule: fix object listing when no table is given")
> Make sure tests/shell/testcases/listing/0014objects_0 passes.
manually checked, deferred for integration bug 1754047

> * 0f44d4f627535 ("proto: fix icmp/icmpv6 code datatype")
> Make sure tests/py/ip/icmp.t and tests/py/ip6/icmpv6.t pass.
unable to execute tests/py/ because of broken deps. integration bug 1754047

> * 5b35fb3132b1f ("evaluate: throw distinct error if map exists but contains
automated

> * 1018eae77176c ("parser: bail out on incorrect burst unit")
> Make sure tests/py/any/limit.t passes.
unable to execute tests/py/ because of broken deps. integration bug 1754047

> * b338244abc7f0 ("src: fix netdev family device name parsing")
automated as part of 'stolen verdict' for bbbed9f3175c5

> * a0da4c5bbf0d7 ("libnftables: Print errors before freeing commands")
sanity, not covered

> * afd1ad6f68680 ("segtree: fix crash when debug mode is active")
automated

> * d3cace2660925 ("parser_bison: no need for statement separator for ct
> object commands")
automated

> * e07be57df3f51 ("ct: use nft_print() instead of printf()")
sanity, not covered

> * 4d97f0a4eebd2 ("parser_bison: type_identifier string memleak")
> * 4ac11b890fe87 ("src: missing destroy function in statement definitions")
sanity, not covered

> * 760a8bab07ade ("tests: shell: validate too deep jumpstack from basechain")
> Make sure tests/shell/testcases/chains/0002jumps_1 passes.
manually checked, deferred for integration bug 1754047

> * 276c452e47c5e ("netlink: remove markup json parsing code")
sanity, not covered

> * 1dc9be8445265 ("rule: limit: don't print default burst value")
> Make sure tests/shell/testcases/sets/0026named_limit_0 passes.
manually checked, deferred for integration bug 1754047

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2019:3659