Split test in two, one for interval sets and another with concatenation
+ intervals, so at least intervals are tested in older kernels with no
pipapo support.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Wed, 8 Nov 2023 18:22:20 +0000 (19:22 +0100)]
netlink: fix buffer size for user data in netlink_delinearize_chain()
The correct define is NFTNL_UDATA_CHAIN_MAX and not NFTNL_UDATA_OBJ_MAX.
In current libnftnl, they both are defined as 1, so (with current libnftnl)
there is no difference.
Fixes: 702ac2b72c0e ("src: add comment support for chains") Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Tue, 24 Oct 2023 09:57:10 +0000 (11:57 +0200)]
src: remove xfree() and use plain free()
xmalloc() (and similar x-functions) are used for allocation. They wrap
malloc()/realloc() but will abort the program on ENOMEM.
The meaning of xmalloc() is that it wraps malloc() but aborts on
failure. I don't think x-functions should have the notion, that this
were potentially a different memory allocator that must be paired
with a particular xfree().
Even if the original intent was that the allocator is abstracted (and
possibly not backed by standard malloc()/free()), then that doesn't seem
a good idea. Nowadays libc allocators are pretty good, and we would need
a very special use cases to switch to something else. In other words,
it will never happen that xmalloc() is not backed by malloc().
Also there were a few places, where a xmalloc() was already "wrongly"
paired with free() (for example, iface_cache_release(), exit_cookie(),
nft_run_cmd_from_buffer()).
Or note how pid2name() returns an allocated string from fscanf(), which
needs to be freed with free() (and not xfree()). This requirement
bubbles up the callers portid2name() and name_by_portid(). This case was
actually handled correctly and the buffer was freed with free(). But it
shows that mixing different allocators is cumbersome to get right. Of
course, we don't actually have different allocators and whether to use
free() or xfree() makes no different. The point is that xfree() serves
no actual purpose except raising irrelevant questions about whether
x-functions are correctly paired with xfree().
Note that xfree() also used to accept const pointers. It is bad to
unconditionally for all deallocations. Instead prefer to use plain
free(). To free a const pointer use free_const() which obviously wraps
free, as indicated by the name.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Tue, 24 Oct 2023 09:57:09 +0000 (11:57 +0200)]
src: add free_const() and use it instead of xfree()
Almost everywhere xmalloc() and friends is used instead of malloc().
This is almost everywhere paired with xfree().
xfree() has two problems. First, it brings the wrong notion that
xmalloc() should be paired with xfree(), as if xmalloc() would not use
the plain malloc() allocator. In practices, xfree() just wraps free(),
and it wouldn't make sense any other way. xfree() should go away. This
will be addressed in the next commit.
The problem addressed by this commit is that xfree() accepts a const
pointer. Paired with the practice of almost always using xfree() instead
of free(), all our calls to xfree() cast away constness of the pointer,
regardless whether that is necessary. Declaring a pointer as const
should help us to catch wrong uses. If the xfree() function always casts
aways const, the compiler doesn't help.
There are many places that rightly cast away const during free. But not
all of them. Add a free_const() macro, which is like free(), but accepts
const pointers. We should always make an intentional choice whether to
use free() or free_const(). Having a free_const() macro makes this very
common choice clearer, instead of adding a (void*) cast at many places.
Note that we now pair xmalloc() allocations with a free() call (instead
of xfree(). That inconsistency will be resolved in the next commit.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Tue, 24 Oct 2023 09:57:08 +0000 (11:57 +0200)]
gmputil: add nft_gmp_free() to free strings from mpz_get_str()
mpz_get_str() (with NULL as first argument) will allocate a buffer using
the allocator functions (mp_set_memory_functions()). We should free
those buffers with the corresponding free function.
Add nft_gmp_free() for that and use it.
The name nft_gmp_free() is chosen because "mini-gmp.c" already has an
internal define called gmp_free(). There wouldn't be a direct conflict,
but using the same name is confusing. And maybe our own defines should
have a clear nft prefix.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
evaluate: place byteorder conversion before rshift in payload expressions
Use the key from the evaluation context to perform the byteorder
conversion in case that this expression is used for lookups and updates
on explicit sets.
Simple matches on values and implicit sets rely on the binary transfer
mechanism to propagate the shift to the constant, no explicit byteorder
is required in such case.
Fixes: 668c18f67203 ("evaluate: place byteorder conversion before rshift in payload statement") Reported-by: Florian Westphal <fw@strlen.de> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
evaluate: reset statement length context only for set mappings
map expression (which is used a key to look up for the mapping) needs to
consider the statement length context, otherwise incorrect bytecode is
generated when {ct,meta} statement is generated.
# nft -f - <<EOF
add table ip6 t
add chain ip6 t c
add map ip6 t mapv6 { typeof ip6 dscp : meta mark; }
EOF
# nft -d netlink add rule ip6 t c meta mark set ip6 dscp map @mapv6
ip6 t c
[ payload load 2b @ network header + 0 => reg 1 ]
[ bitwise reg 1 = ( reg 1 & 0x0000c00f ) ^ 0x00000000 ]
... missing byteorder conversion here before shift ...
[ bitwise reg 1 = ( reg 1 >> 0x00000006 ) ]
[ lookup reg 1 set mapv6 dreg 1 ]
[ meta set mark with reg 1 ]
Reset statement length context only for the mapping side for the
elements in the set.
Fixes: edecd58755a8 ("evaluate: support shifts larger than the width of the left operand") Reported-by: Brian Davidson <davidson.brian@gmail.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
meta: fix hour decoding when timezone offset is negative
Brian Davidson says:
meta hour rules don't display properly after being created when the
hour is on or after 00:00 UTC. The netlink debug looks correct for
seconds past midnight UTC, but displaying the rules looks like an
overflow or a byte order problem. I am in UTC-0400, so today, 20:00
and later exhibits the problem, while 19:00 and earlier hours are
fine.
meta.c only ever worked when the delta to UTC is positive.
We need to add in case the second counter turns negative after
offset adjustment.
Also add a test case for this.
Fixes: f8f32deda31d ("meta: Introduce new conditions 'time', 'day' and 'hour'") Reported-by: Brian Davidson <davidson.brian@gmail.com> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Thu, 2 Nov 2023 13:48:10 +0000 (14:48 +0100)]
tproxy: Drop artificial port printing restriction
It does not make much sense to omit printing the port expression if it's
not a value expression: On one hand, input allows for more advanced
uses. On the other, if it is in-kernel, best nft can do is to try and
print it no matter what. Just ignoring ruleset elements can't be
correct.
Fixes: 2be1d52644cf7 ("src: Add tproxy support") Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1721 Signed-off-by: Phil Sutter <phil@nwl.cc> Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>
This has several problems, which we an avoid with a single Makefile:
- recursive-make is harder to maintain and understand as a whole.
Recursive-make makes sense, when there are truly independent
sub-projects. Which is not the case here. The project needs to be
considered as a whole and not one directory at a time. When
we add unit tests (which we should), those would reside in separate
directories but have dependencies between directories. With a single
Makefile, we see all at once. The build setup has an inherent complexity,
and that complexity is not necessarily reduced by splitting it into more files.
On the contrary it helps to have it all in once place, provided that it's
sensibly structured, named and organized.
- typing `make` prints irrelevant "Entering directory" messages. So much
so, that at the end of the build, the terminal is filled with such
messages and we have to scroll to see what even happened.
- with recursive-make, during build we see:
make[3]: Entering directory '.../nftables/src'
CC meta.lo
meta.c:13:2: error: #warning hello test [-Werror=cpp]
13 | #warning hello test
| ^~~~~~~
With a single Makefile we get
CC src/meta.lo
src/meta.c:13:2: error: #warning hello test [-Werror=cpp]
13 | #warning hello test
| ^~~~~~~
This shows the full filename -- assuming that the developer works from
the top level directory. The full name is useful, for example to
copy+paste into the terminal.
- single Makefile is also faster:
$ make && perf stat -r 200 -B make -j
I measure 35msec vs. 80msec.
- recursive-make limits parallel make. You have to craft the SUBDIRS= in
the correct order. The dependencies between directories are limited,
as make only sees "LDADD = $(top_builddir)/src/libnftables.la" and
not the deeper dependencies for the library.
- I presume, some people like recursive-make because of `make -C $subdir`
to only rebuild one directory. Rebuilding the entire tree is already very
fast, so this feature seems not relevant. Also, as dependency handling
is limited, we might wrongly not rebuild a target. For example,
make check
touch src/meta.c
make -C examples check
does not rebuild "examples/nft-json-file".
What we now can do with single Makefile (and better than before), is
`make examples/nft-json-file`, which works as desired and rebuilds all
dependencies.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
Not sure why this starts to fail. But arguably the command line
arguments were wrong. Fix it, we need a pseudo name for the device.
Fixes: df6f1a3e0803 ("tests/shell: bind mount private /var/run/netns in test container") Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
Florian Westphal [Tue, 24 Oct 2023 10:37:47 +0000 (12:37 +0200)]
check-tree.sh: check and flag /bin/sh usage
Almost all shell tests use /bin/bash already.
In some cases we've had shell tests use /bin/sh, but still having
a bashism. This causes failures on systems where sh is dash or another,
strict bourne shell.
Thomas Haller [Mon, 23 Oct 2023 17:00:47 +0000 (19:00 +0200)]
parser_bison: fix length check for ifname in ifname_expr_alloc()
IFNAMSIZ is 16, and the allowed byte length of the name is one less than
that. Fix the length check and adjust a test for covering the longest
allowed interface name.
This is obviously a change in behavior, because previously interface
names with length 16 were accepted and were silently truncated along the
way. Now they are rejected as invalid.
Fixes: fa52bc225806 ("parser: reject zero-length interface names") Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 23 Oct 2023 17:00:46 +0000 (19:00 +0200)]
tests/shell: cover long interface name in "0042chain_variable_0" test
IFNAMSIZ is 16. Adjust "0042chain_variable_0" to use an interface name
with the maximum allowed bytes length.
Instead of adding an entirely different test, adjust an existing one to
use another interface name. The aspect for testing for a long interface
name is not special enough, to warrant a separate test. We can cover it
by extending an existing test.
Note that the length check in "parser_bison.y" is wrong. The test checks
still for the wrong behavior and that "d23456789012345x" is accepted.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 23 Oct 2023 16:13:16 +0000 (18:13 +0200)]
tools: reject unexpected files in "tests/shell/testcases/" with "check-tree.sh"
"check-tree.sh" does consistency checks on the source tree. Extend
the check to flag more unexpected files. We don't want to accidentally
have left over files.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 23 Oct 2023 16:13:15 +0000 (18:13 +0200)]
tests/shell: inline input data in "single_anon_set" test
The file "optimizations/dumps/single_anon_set.nft.input" was laying
around, and it was unclear how it was used.
Let's extend "check-patch.sh" to flag all unused files. But the script
cannot understand how "single_anon_set.nft.input" is used (aside allow
listing it).
Instead, inline the script to keep it inside the test (script).
We still write the data to a separate file and don't use `nft -f -`
(because reading stdin uses a different code path we want to cover).
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 23 Oct 2023 13:38:18 +0000 (15:38 +0200)]
tests/shell: test for maximum length of "comment" in "comments_objects_0"
The comment length is limited to NFTNL_UDATA_COMMENT_MAXLEN. Test for
that.
Adjust an existing test for that.
Also rename $EXPECTED to $RULESET. We don't compare the value of
$EXPECTED against the actually configured rules. It also wouldn't work,
because the input is not normalized and wouldn't match. It also isn't
necessary, because there is a .nft dump file.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
This is an inconsistency. The test should have either a .nft or a
.nodump file. "./tools/check-tree.sh" enforces that and will in the
future run by `make check`.
Fixes: 22fab8681a50 ("parser_bison: Fix for broken compatibility with older dumps") Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 16 Oct 2023 19:43:08 +0000 (21:43 +0200)]
tests/shell: honor NFT_TEST_VERBOSE_TEST variable to debug tests via `bash -x`
It can be cumbersome to debug why a test fails. Our tests are just shell
scripts, which for the most part don't print much. That is good, but for
debugging, it can be useful to run the test via `bash -x`. Previously,
we would just patch the source file while debugging.
Add an option "-x" and NFT_TEST_VERBOSE_TEST=y environment variable. If set,
"test-wrapper.sh" will check whether the shebang is "#!/bin/bash" and add
"-x" to the command line.
While at it, let test-wrapper.sh also log a line like
Command: $CMD
With this, we see in the log the command that was run, and how
NFT_TEST_VERBOSE_TEST may have affected it. This is anyway useful,
because many tests don't print anything at all, and we end up with an
empty "testout.log". Empty files are cumbersome, e.g. I like to use
`grep -R ^` to show the content of all files, which does not show empty
files. Ensuring that something is always written is desirable.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
Phil Sutter [Thu, 19 Oct 2023 16:40:04 +0000 (18:40 +0200)]
parser_bison: Fix for broken compatibility with older dumps
Commit e6d1d0d611958 ("src: add set element multi-statement
support") changed the order of expressions and other state attached to set
elements are expected in input. This broke parsing of ruleset dumps
created by nft commands prior to that commit.
Restore compatibility by also accepting the old ordering.
Fixes: e6d1d0d611958 ("src: add set element multi-statement support") Signed-off-by: Phil Sutter <phil@nwl.cc>
evaluate: validate maximum log statement prefix length
Otherwise too long string overruns the log prefix buffer.
Fixes: e76bb3794018 ("src: allow for variables in the log prefix string") Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1714 Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 16 Oct 2023 13:30:10 +0000 (15:30 +0200)]
tests/shell: use bash instead of /bin/sh for tests
All tests under "tests/shell" are shell scripts with shebang /bin/bash
or /bin/sh. This may seem expected, since these tests are under
"tests/shell" directory, but any executable file would work.
Anyway. The vast majority of the tests has "#!/bin/bash" as shebang.
A few tests had "#!/bin/sh" or "#!/bin/sh -e". Unify this and always use bash.
Since we anyway require bash, this is not a limitation.
Also, if we know that this is a bash script (by parsing the shebang), we
can let the test wrapper pass "-x" to the script. The next commit will
do that, and it is nicer if the shebangs are all uniform.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
This is an inconsistency. The test should have either a .nft or a
.nodump file. "./tools/check-tree.sh" enforces that and will in the
future run by `make check`.
Fixes: 74cf3d16d8e9 ('tests: shell: add vlan match test case') Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
Florian Westphal [Fri, 13 Oct 2023 10:37:21 +0000 (12:37 +0200)]
evaluate: suggest != in negation error message
when I run sudo nft insert rule filter FORWARD iifname "ens2f1" ip saddr not @ip_macs counter drop comment \" BLOCK ALL NON REGISTERED IP/MACS \"
I get: Error: negation can only be used with singleton bitmask values
And even I did not spot the problem immediately.
I don't think "not" should have been added, its easily confused with
"not equal"/"neq"/!= and hides that this is allegedly a binop.
At least *mention* that the commandline is asking for a binary
operation here and suggest "!=".
Thomas Haller [Fri, 6 Oct 2023 09:42:20 +0000 (11:42 +0200)]
tests/shell: add "-S|--setup-host" option to set sysctl for rootless tests
Most tests can run just fine without root. A few of them will fail if
/proc/sys/net/core/{wmem_max,rmem_max} is too small (as it is by default
on the host).
The easy workaround is to bump those limits once. This has to be
repeated after each reboot.
Doing that manually (every time) is cumbersome. Add a "--setup-host"
option for that.
Usage:
$ sudo ./tests/shell/run-tests.sh -S
Setting up host for running as rootless (requires root).
echo 4096000 > /proc/sys/net/core/rmem_max (previous value 100000)
echo 4096000 > /proc/sys/net/core/wmem_max (previous value 100000)
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Fri, 6 Oct 2023 09:42:18 +0000 (11:42 +0200)]
tests/shell: mount all of "/var/run" in "test-wrapper.sh"
After reboot, "/var/run/netns" does not exist before we run the first
`ip netns add` command. Previously, "test-wrapper.sh" would mount a
tmpfs on that directory, but that fails, if the directory doesn't exist.
You will notice this, by deleting /var/run/netns (which only root can
delete or create, and which is wiped on reboot).
Instead, mount all of "/var/run". Then we can also create /var/run/netns
directory.
This means, any other content from /var/run is hidden too. That's
probably desirable, because it means we don't depend on stuff that
happens to be there. If we would require other content in /var/run, then
the test runner needs to be aware of the requirement and ensure it's
present. But best is just to not require anything. It's only iproute2
which insists on /var/run/netns.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
doc: remove references to timeout in reset command
After Linux kernel's patch ("netfilter: nf_tables: do not refresh
timeout when resetting element") timers are not reset anymore, update
documentation to keep this in sync.
Fixes: 83e0f4402fb7 ("Implement 'reset {set,map,element}' commands") Acked-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
The function print_times() skips any time elements which are zero, so
output may lack the ms part. Adjust the sed call dropping anything but
the minutes value to not fail in that case.
Reported-by: Pablo Neira Ayuso <pablo@netfilter.org> Fixes: 255ec36a11525 ("tests: shell: Stabilize sets/reset_command_0 test") Signed-off-by: Phil Sutter <phil@nwl.cc>
Phil Sutter [Thu, 28 Sep 2023 16:19:37 +0000 (18:19 +0200)]
tests: shell: Fix for failing nft-f/sample-ruleset
For whatever reason, my system lacks an entry for 'sip' in
/etc/services. Assuming the service name is not relevant to the test,
just replace it by the respective port number.
tests: shell: fix spurious errors in sets/0036add_set_element_expiration_0
A number of changes to fix spurious errors:
- Add seconds as expiration, otherwise 14m59 reports 14m in minute
granularity, this ensures suficient time in a very slow environment with
debugging instrumentation.
- Provide expected output.
- Update sed regular expression to make 'ms' optional and use -E mode.
Fixes: adf38fd84257 ("tests: shell: use minutes granularity in sets/0036add_set_element_expiration_0") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
netlink_linearize: skip set element expression in map statement key
This fix is similar to 22d201010919 ("netlink_linearize: skip set element
expression in set statement key") to fix map statement.
netlink_gen_map_stmt() relies on the map key, that is expressed as a set
element. Use the set element key instead to skip the set element wrap,
otherwise get_register() abort execution:
The dynamic flag is not exported via JSON, this triggers spurious
ENOTSUPP errors when restoring rulesets in JSON with dynamic flags
set on.
Fixes: 6e45b102650a2 ("nft: set: print dynamic flag when set") Acked-by: Phil Sutter <phil@nwl.cc> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Wed, 20 Sep 2023 14:26:08 +0000 (16:26 +0200)]
expression: cleanup expr_ops_by_type() and handle u32 input
Make fewer assumptions about the underlying integer type of the enum.
Instead, be clear about where we have an untrusted uint32_t from netlink
and an enum. Rename expr_ops_by_type() to expr_ops_by_type_u32() to make
this clearer. Later we might make the enum as packed, when this starts
to matter more.
Also, only the code path expr_ops() wants strict validation and assert
against valid enum values. Move the assertion out of
__expr_ops_by_type(). Then expr_ops_by_type_u32() does not need to
duplicate the handling of EXPR_INVALID. We still need to duplicate the
check against EXPR_MAX, to ensure that the uint32_t value can be cast to
an enum value.
[ Remove cast on EXPR_MAX. --pablo ]
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Phil Sutter [Wed, 13 Sep 2023 18:53:41 +0000 (20:53 +0200)]
parser_json: Wrong check in json_parse_ct_timeout_policy()
The conditional around json_unpack() was meant to accept a missing
policy attribute. But the accidentally inverted check made the function
either ignore a given policy or access uninitialized memory.
Parts of the code were unsafe (parsing 'I' format into uint32_t), the
rest just plain wrong (parsing 'o' format into char *tmp). Introduce a
temporary int variable to parse into.
Phil Sutter [Wed, 13 Sep 2023 18:32:37 +0000 (20:32 +0200)]
parser_json: Catch wrong "reset" payload
The statement happily accepted any valid expression as payload and
assumed it to be a tcpopt expression (actually, a special case of
exthdr). Add a check to make sure this is the case.
Standard syntax does not provide this flexibility, so no need to have
the check there as well.
tests: shell: add feature probe for sets with more than one element
Kernels < 5.11 can handle only one expression per element, e.g.
its possible to attach a counter per key, or a rate limiter,
or a quota, but not two at the same time.
Add a probe file and skip the relevant tests if the feature is absent.
Thomas Haller [Mon, 18 Sep 2023 20:27:07 +0000 (22:27 +0200)]
tests/shell: honor NFT_TEST_FAIL_ON_SKIP variable to fail on any skipped tests
The test suite should pass with various kernels and build
configurations. Of course, that means, that some tests will be
gracefully skipped, and we don't treat that as an overall failure.
However, it should be possible to run a specific kernel (net-next?) and
build configuration, where we expect that all tests pass.
Add an option to fail the run, if any tests were skipped. This is to
ensure that we don't have broken tests that never pass.
This will make more sense with automated CI is running, to enable on a
test system and ensure that at least on that system, all tests pass.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
Thomas Haller [Wed, 20 Sep 2023 20:09:19 +0000 (22:09 +0200)]
datatype: return const pointer from datatype_get()
"struct datatype" is for the most part immutable, and most callers deal
with const pointers. That's why datatype_get() accepts a const pointer
to increase the reference count (mutating the refcnt field).
It should also return a const pointer. In fact, all callers are fine
with that already.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Wed, 20 Sep 2023 14:26:10 +0000 (16:26 +0200)]
proto: add missing proto_definitions for PROTO_DESC_GENEVE
While at it, make proto_definitions const. For global variables, this
allows the linker to mark the memory as read only. It's just good to do
by default.
Fixes: 156d22654003 ("src: add geneve matching support") Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
datatype: initialize TYPE_CT_EVENTBIT slot in datatype array
Matching on ct event makes no sense since this is mostly used as
statement to globally filter out ctnetlink events, but do not crash
if it is used from concatenations.
Add the missing slot in the datatype array so this does not crash.
Fixes: 2595b9ad6840 ("ct: add conntrack event mask support") Reported-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
datatype: initialize TYPE_CT_LABEL slot in datatype array
Otherwise, ct label with concatenations such as:
table ip x {
chain y {
ct label . ct mark { 0x1 . 0x1 }
}
}
crashes:
../include/datatype.h:196:11: runtime error: member access within null pointer of type 'const struct datatype'
AddressSanitizer:DEADLYSIGNAL
=================================================================
==640948==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc970d3199b bp 0x7fffd1f20560 sp 0x7fffd1f20540 T0)
==640948==The signal is caused by a READ memory access.
==640948==Hint: address points to the zero page.
sudo #0 0x7fc970d3199b in datatype_equal ../include/datatype.h:196
Fixes: 2fcce8b0677b ("ct: connlabel matching support") Reported-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 18 Sep 2023 19:59:24 +0000 (21:59 +0200)]
tests/shell: run `nft --check` on persisted dump files
"nft --check" will trigger a rollback in kernel. The existing dump files
might hit new code paths. Take the opportunity to call the command on
the existing files.
And alternative would be to write a separate tests, that iterates over
all files. However, then we can only run all the commands sequentially
(unless we do something smart). That might be slower than the
opportunity to run the checks in parallel. More importantly, it would be
nice if the check for the dump file is clearly tied to the file's test.
So run it right after the test, from the test wrapper.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
A library should not restrict being used by multiple threads or make
assumptions about how it's being used. Hence a "init_once" pattern
without no locking is racy, a code smell and should be avoided.
Note that libxtables is full of global variables and when linking against
it, libnftables cannot be used from multiple threads either. That is not
easy to fix.
Move the ugliness of "init_once" away from nft_ctx_new(), so that the
problem is concentrated closer to libxtables.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Tue, 19 Sep 2023 12:36:16 +0000 (14:36 +0200)]
libnftables: drop gmp_init() and mp_set_memory_functions()
Setting global handles for libgmp via mp_set_memory_functions() is very
ugly. When we don't use mini-gmp, then potentially there are other users
of the library in the same process, and every process fighting about the
allocation functions is not gonna work.
It also means, we must not reset the allocation functions after somebody
already allocated GMP data with them. Which we cannot ensure, as we
don't know what other parts of the process are doing.
It's also unnecessary. The default allocation functions for gmp and
mini-gmp already abort the process on allocation failure ([1], [2]),
just like our xmalloc().
3975430b12d9 ("src: expand table command before evaluation") moved
ruleset expansion before evaluation, except for sets and maps. For
sets and maps there is still a post_expand() phase.
This patch moves sets and map expansion to allocate an independent
CMD_OBJ_SETELEMS command to add elements to named set and maps which is
evaluated, this consolidates the ruleset expansion to happen always
before the evaluation step for all objects, except for anonymous sets
and maps.
This approach avoids an interference with the set interval code which
detects overlaps and merges of adjacents ranges. This set interval
routine uses set->init to maintain a cache of existing elements. Then,
the post_expand() phase incorrectly expands set->init cache and it
triggers a bogus ENOENT errors due to incorrect bytecode (placing
element addition before set creation) in combination with user declared
sets using the flat syntax notation.
Since the evaluation step (coming after the expansion) creates
implicit/anonymous sets and maps, those are not expanded anymore. These
anonymous sets still need to be evaluated from set_evaluate() path and
the netlink bytecode generation path, ie. do_add_set(), needs to deal
with anonymous sets.
Note that, for named sets, do_add_set() does not use set->init. Such
content is part of the existing cache, and the CMD_OBJ_SETELEMS command
is responsible for adding elements to named sets.
Fixes: 3975430b12d9 ("src: expand table command before evaluation") Reported-by: Jann Haber <jannh@selfnet.de> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
evaluate: fix memleak in prefix evaluation with wildcard interface name
The following ruleset:
table ip x {
chain y {
meta iifname { abcde*, xyz }
}
}
triggers the following memleak:
==6871== 16 bytes in 1 blocks are definitely lost in loss record 1 of 1
==6871== at 0x483877F: malloc (vg_replace_malloc.c:307)
==6871== by 0x48AD898: xmalloc (utils.c:37)
==6871== by 0x4BC8B22: __gmpz_init2 (in /usr/lib/x86_64-linux-gnu/libgmp.so.10.4.1)
==6871== by 0x4887E67: constant_expr_alloc (expression.c:424)
==6871== by 0x488EF1F: expr_evaluate_prefix (evaluate.c:1138)
==6871== by 0x488EF1F: expr_evaluate (evaluate.c:2725)
==6871== by 0x488E76D: expr_evaluate_set_elem (evaluate.c:1662)
==6871== by 0x488E76D: expr_evaluate (evaluate.c:2739)
==6871== by 0x4891033: list_member_evaluate (evaluate.c:1454)
==6871== by 0x488E2B6: expr_evaluate_set (evaluate.c:1757)
==6871== by 0x488E2B6: expr_evaluate (evaluate.c:2737)
==6871== by 0x48910D0: elems_evaluate (evaluate.c:4605)
==6871== by 0x4891432: set_evaluate (evaluate.c:4711)
==6871== by 0x48915BC: implicit_set_declaration (evaluate.c:122)
==6871== by 0x488F18A: expr_evaluate_relational (evaluate.c:2503)
==6871== by 0x488F18A: expr_evaluate (evaluate.c:2745)
expr_evaluate_prefix() calls constant_expr_alloc() which have already
called mpz_init2(), the second call to mpz_init2() overlaps the existing
mpz_t data memory area.
Remove extra mpz_init2() call to fix this memleak.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Thu, 14 Sep 2023 14:09:50 +0000 (16:09 +0200)]
netlink: fix leaking typeof_expr_data/typeof_expr_key in netlink_delinearize_set()
There are various code paths that return without freeing typeof_expr_data
and typeof_expr_key. It's not at all obvious, that there isn't a leak
that way. Quite possibly there is a leak. Fix it, or at least make the
code more obviously correct.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thomas Haller [Mon, 18 Sep 2023 19:59:23 +0000 (21:59 +0200)]
tests/shell: simplify collecting error result in "test-wrapper.sh"
The previous pattern was unnecessarily confusing.
The "$rc_{dump,valgrind,tainted}" variable should only remember whether
that particular check failed, not the overall exit code of the test
wrapper.
Otherwise, if you want to know in which case the wrapper exits with code
122, you have to oddly follow the rc_valgrind variable.
This change will make more sense, when we add another such variable, but
which will be assigned the non-zero value at multiple places. Assigning
there the exit code of the wrapper, duplicates the places where the
condition maps to the exit code.
Signed-off-by: Thomas Haller <thaller@redhat.com> Signed-off-by: Florian Westphal <fw@strlen.de>
projects / thirdparty / nftables.git / log
