nft prints non-existent cgroup names as the raw id using PRIu64, but cgroupv2_type_parse() only stats /sys/fs/cgroup/, so the listing does not load back: # nft list set ip t s table ip t { set s { type cgroupsv2 elements = { 50834 } } } # nft delete element ip t s { 50834 } Error: cgroupv2 path fails: No such file or directory The element cannot be deleted by key once its cgroup is gone, only flushed with the set, and a dump does not restore. The json dump has the same problem, and the stale id has been visible in the wild since 2022 (see Link). tests/shell/testcases/packetpath/cgroupv2 already works around this in cleanup(). Fall back to integer_type_parse() when the path does not resolve, as boolean_type_parse() already does. The path lookup stays first, so a cgroup named as a number still resolves as a path. Improving the integer parser is left for a follow-up as discussed in v1. Fixes: 38228087252c ("src: add cgroupsv2 support") Link: https://lore.kernel.org/netfilter-devel/fabde324-383a-622c-7e69-32c9b2d06191@gmail.com/ Signed-off-by: Avinash Duduskar --- Changes since v2 (https://lore.kernel.org/netfilter-devel/20260729173748.1033454-1-avinash.duduskar@gmail.com/): - return the error from integer_type_parse() instead of discarding it (Pablo) Changes since v1 (https://lore.kernel.org/netfilter-devel/20260727082427.740789-1-avinash.duduskar@gmail.com/): - drop the strict decimal parser, delegate to integer_type_parse() (Pablo, Phil) - json test arm uses a here-string instead of a tempfile (Phil) - shorter commit message and test src/datatype.c | 4 +- .../shell/testcases/parsing/cgroupv2_stale_id | 128 ++++++++++++++++++ .../parsing/dumps/cgroupv2_stale_id.json-nft | 11 ++ .../parsing/dumps/cgroupv2_stale_id.nft | 0 4 files changed, 141 insertions(+), 2 deletions(-) create mode 100755 tests/shell/testcases/parsing/cgroupv2_stale_id create mode 100644 tests/shell/testcases/parsing/dumps/cgroupv2_stale_id.json-nft create mode 100644 tests/shell/testcases/parsing/dumps/cgroupv2_stale_id.nft diff --git a/src/datatype.c b/src/datatype.c index 4dbca16e..5025b276 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -1665,9 +1665,9 @@ static struct error_record *cgroupv2_type_parse(struct parse_ctx *ctx, SYSFS_CGROUPSV2_PATH, sym->identifier); cgroupv2_path[sizeof(cgroupv2_path) - 1] = '\0'; + /* the listing prints a raw id once the path is gone */ if (stat(cgroupv2_path, &st) < 0) - return error(&sym->location, "cgroupv2 path fails: %s", - strerror(errno)); + return integer_type_parse(ctx, sym, res); ino = st.st_ino; *res = constant_expr_alloc(&sym->location, &cgroupv2_type, diff --git a/tests/shell/testcases/parsing/cgroupv2_stale_id b/tests/shell/testcases/parsing/cgroupv2_stale_id new file mode 100755 index 00000000..7f1833a8 --- /dev/null +++ b/tests/shell/testcases/parsing/cgroupv2_stale_id @@ -0,0 +1,128 @@ +#!/bin/bash + +# A cgroupsv2 element has to stay addressable once its cgroup is gone: the +# listing prints the raw id, so that id has to be accepted back. + +CGROUP="/sys/fs/cgroup/nft-stale-$$" +# numeric on purpose: a cgroup named as a number must still resolve as a path +CGNUM="/sys/fs/cgroup/$$" + +cleanup() +{ + $NFT delete table t 2>/dev/null + rmdir "$CGROUP" 2>/dev/null + rmdir "$CGNUM" 2>/dev/null +} +trap cleanup EXIT + +if [ ! -w /sys/fs/cgroup ]; then + echo "cgroup filesystem not writable" + exit 77 +fi + +# -w alone passes on a v1 or hybrid layout, where everything below tests nothing +if [ "$(stat -f --printf=%T /sys/fs/cgroup)" != "cgroup2fs" ]; then + echo "not a cgroupv2 mount" + exit 77 +fi + +if ! mkdir "$CGROUP" 2>/dev/null; then + # unprivileged and left over from a dead run are different facts + [ -d "$CGROUP" ] && { + echo "E: $CGROUP already exists" >&2 + exit 1 + } + echo "cannot create a cgroup" + exit 77 +fi +name="${CGROUP##*/}" +id=$(stat --printf=%i "$CGROUP") + +$NFT add table t || exit 1 +$NFT add set t s '{ type cgroupsv2; }' || exit 1 +$NFT add element t s "{ \"$name\" }" || exit 1 + +rmdir "$CGROUP" || exit 1 + +# Another cgroup can take this inode between the rmdir and the listing, which +# resolves it back to a name. Narrow, and not closable from here. +$NFT list set t s | grep -qw "$id" || { + echo "E: listing does not show the stale cgroup id $id" >&2 + $NFT list set t s >&2 + exit 1 +} + +$NFT delete element t s "{ $id }" || { + echo "E: cannot delete a cgroupsv2 element by the id that was listed" >&2 + exit 1 +} + +$NFT list set t s | grep -qw "$id" && { + echo "E: element still present after delete" >&2 + exit 1 +} + +$NFT add element t s "{ $id }" || exit 1 + +# json serialises the id as a string, so it reaches the same parser. The harness +# round trip at exit sees an empty ruleset here, so do it inline, scoped to this +# table. +if [ "$NFT_TEST_HAVE_json" != n ] && $NFT -j list tables >/dev/null 2>&1; then + out=$($NFT -j list table t) || exit 1 + $NFT delete table t || exit 1 + $NFT -j -f - <<< "$out" || { + echo "E: a json dump holding a stale cgroupsv2 id does not reload" >&2 + exit 1 + } + $NFT list set t s | grep -qw "$id" || { + echo "E: the id did not survive the json round trip" >&2 + exit 1 + } +else + echo "I: no json support, skipping the json reload check" +fi + +$NFT flush set t s || exit 1 + +# a name that is neither a path nor an integer must still fail, and a value +# wider than the 64-bit key must be rejected by evaluation +for bogus in "nft-does-not-exist" "12abc" "18446744073709551616"; do + $NFT add element t s "{ \"$bogus\" }" 2>/dev/null && { + echo "E: accepted \"$bogus\" as a cgroupsv2 id" >&2 + $NFT list set t s >&2 + exit 1 + } +done + +# each add above also "passes" if it fails for an unrelated reason, so check +# nothing was stored, without a pipeline that goes vacuous when list fails +out=$($NFT list set t s) || exit 1 +case "$out" in +*elements*) + echo "E: something was stored despite every add failing" >&2 + echo "$out" >&2 + exit 1 + ;; +esac + +# The path has to win for a cgroup named as a number. While the directory +# exists both readings print the same, so remove it before asserting. +mkdir "$CGNUM" || exit 1 +numino=$(stat --printf=%i "$CGNUM") +$NFT add element t s "{ \"$$\" }" || { + echo "E: cannot add a cgroup whose name is a number" >&2 + exit 1 +} +rmdir "$CGNUM" || exit 1 +if [ "$numino" = "$$" ]; then + echo "I: cgroup $$ happens to have inode $$, cannot tell the two apart" +else + $NFT list set t s | grep -qw "$numino" || { + echo "E: \"$$\" was taken as an id, not resolved as a path" >&2 + $NFT list set t s >&2 + exit 1 + } +fi +$NFT flush set t s || exit 1 + +exit 0 diff --git a/tests/shell/testcases/parsing/dumps/cgroupv2_stale_id.json-nft b/tests/shell/testcases/parsing/dumps/cgroupv2_stale_id.json-nft new file mode 100644 index 00000000..546cc597 --- /dev/null +++ b/tests/shell/testcases/parsing/dumps/cgroupv2_stale_id.json-nft @@ -0,0 +1,11 @@ +{ + "nftables": [ + { + "metainfo": { + "version": "VERSION", + "release_name": "RELEASE_NAME", + "json_schema_version": 1 + } + } + ] +} diff --git a/tests/shell/testcases/parsing/dumps/cgroupv2_stale_id.nft b/tests/shell/testcases/parsing/dumps/cgroupv2_stale_id.nft new file mode 100644 index 00000000..e69de29b base-commit: 31fb7af1a99d67c87e94b4415e553589b21db2d1 -- 2.55.0