Re: [PATCH v1 net-next 1/5] net: Remove net->is_dying.

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: Remove net->is_dying." says
  net->is_dying "is an alias of !check_net(net)".

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a valid NSID
  of INT_MAX makes the signed cursor in unhash_nsid() overflow, which…
- [Low] This problem wasn't introduced by this patch, but the block
  comment in cleanup_net() right after the loop this patch edits says…

--

net: Remove net->is_dying.

This patch removes the is_dying flag from struct net. cleanup_net() used
to set that flag on the nets in its current kill batch. In unhash_nsid()
the flag check is replaced with check_net(peer), on the grounds that
is_dying is an alias of !check_net(net).

> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index da5f881fbd3b9..0e13de0cd36b6 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -644,7 +644,7 @@ static void unhash_nsid(struct net *last)
>  			int curr_id = id;
>
>  			id++;

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. What
happens here when a netns_ids IDR has an entry at INT_MAX?

rtnl_net_newid() accepts any s32 NETNSA_NSID. alloc_netid() then does:

	min = reqid;
	max = reqid + 1;

For reqid == INT_MAX, max wraps to INT_MIN. idr_alloc() treats end <= 0
as unbounded, so ID INT_MAX gets allocated.

When unhash_nsid() later reaches that entry, id++ wraps to INT_MIN,
whether or not the entry is removed. On the next iteration idr_get_next()
turns the cursor into an unsigned long above INT_MAX. idr_get_next_ul()
finds no slot and leaves the value unchanged, so this fires:

lib/idr.c:idr_get_next() {
	unsigned long id = *nextid;
	void *entry = idr_get_next_ul(idr, &id);

	if (WARN_ON_ONCE(id > INT_MAX))
		return NULL;
	...
}

An unprivileged user can reach this with unshare(CLONE_NEWUSER |
CLONE_NEWNET) followed by RTM_NEWNSID with NETNSA_NSID set to INT_MAX.
The next netns teardown anywhere on the system then hits the WARN. With
panic_on_warn=1 that becomes a panic. No entries get skipped, because
INT_MAX is the last possible ID.

Would it make sense to use an unsigned long cursor with idr_get_next_ul(),
or to break out of the loop after processing INT_MAX?

> -			if (!peer->is_dying)
> +			if (check_net(peer))
>  				continue;

[Severity: Low]
The commit message says:

    It is an alias of !check_net(net), and having two ways to represent
    the same state is confusing.

Is that equivalence exact? is_dying was only set on the nets that
cleanup_net() took from cleanup_list with llist_del_all(). check_net()
is false for every net whose refcount has reached zero.

That set also includes nets queued after this cleanup_net() pass took its
snapshot:

put_net()
  ns_ref_put() reaches 0
  __put_net()
    llist_add(&net->cleanup_list, &cleanup_list)

It also includes nets whose refcount has hit zero but which have not yet
reached llist_add(). Those nets are still on net_namespace_list and still
sit in other nets' netns_ids IDRs.

Before this patch, unhash_nsid() skipped them because is_dying was false,
and their own later cleanup pass handled them. With check_net(peer), the
current pass now calls idr_remove() on them and sends RTM_DELNSID
straight away. That means userspace can see the notification one pass
earlier.

The wider condition does look safe. peernet2id_alloc() goes through
maybe_get_net(peer), and rtnl_net_newid() gets its peer from
get_net_ns_by_pid() or get_net_ns_by_fd(). So a zero-refcount peer can
never be added back to an IDR, and the later pass finds nothing left to
remove.

Could the commit message say that the new check covers more nets than the
old flag did, and explain why that is fine?

Later patches in the series ("net: Track state in ops_undo_list()." and
"ipv4: Batch rt_flush_dev() for dying netns.") describe a dying netns
queued for the next cleanup_net() batch as having !check_net() without
being in the current batch. That also contradicts the "alias" wording.

[ ... ]

> @@ -681,7 +681,6 @@ static void cleanup_net(struct work_struct *work)
>  	llist_for_each_entry(net, net_kill_list, cleanup_list) {
>  		ns_tree_remove(net);
>  		list_del_rcu(&net->list);
> -		net->is_dying = true;
>  	}
>  	/* Cache last net. After we unlock rtnl, no one new net
>  	 * added to net_namespace_list can assign nsid pointer

[Severity: Low]
This isn't a bug introduced by this patch, but the comment says "After we
unlock rtnl". This section of cleanup_net() never takes rtnl. It uses
net_rwsem instead:

	down_write(&net_rwsem);
	llist_for_each_entry(net, net_kill_list, cleanup_list) {
	...
	last = list_last_entry(&net_namespace_list, struct net, list);
	up_write(&net_rwsem);

Should the comment say net_rwsem? The guarantee it describes actually
comes from the maybe_get_net() check in peernet2id_alloc().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927202429.2452589-1-kuniyu%40google.com