Re: [PATCH v1 net-next 2/5] net: Add ->pre_exit_batch() to struct pernet_operations.

[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 new ->pre_exit_batch() hook is undocumented, and the commit
  message describes its scope inaccurately.

--

net: Add ->pre_exit_batch() to struct pernet_operations.

This commit adds a pre_exit_batch() callback to struct pernet_operations.
ops_pre_exit_list() calls it after the per-net pre_exit() calls. The goal
is to let subsystems batch device-related cleanup, such as flushing
uncached routes, across dying namespaces without holding RTNL and before
ops_exit_rtnl_list().

> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index b4247e767d135..58b2601bb8691 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -488,6 +488,7 @@ struct pernet_operations {
>  	 */
>  	int (*init)(struct net *net);
>  	void (*pre_exit)(struct net *net);
> +	void (*pre_exit_batch)(struct list_head *net_exit_list);
>  	void (*exit)(struct net *net);
>  	void (*exit_batch)(struct list_head *net_exit_list);
>  	/* Following method is called with RTNL held. */
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 0e13de0cd36b6..476fbf913bad8 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -160,6 +160,9 @@ static void ops_pre_exit_list(const struct pernet_operations *ops,
>  		list_for_each_entry(net, net_exit_list, exit_list)
>  			ops->pre_exit(net);
>  	}
> +
> +	if (ops->pre_exit_batch)
> +		ops->pre_exit_batch(net_exit_list);

[Severity: Low]
The commit message says the hook is there "To batch such cleanup across
dying netns". Is net_exit_list here always a list of dying namespaces?

ops_pre_exit_list() is also reached through ops_undo_single() from
__unregister_pernet_operations(). That function builds the list from every
live netns, including init_net:

net/core/net_namespace.c:__unregister_pernet_operations() {
    ...
	for_each_net(net)
		list_add_tail(&net->exit_list, &net_exit_list);

	list_del(&ops->list);
	ops_undo_single(ops, &net_exit_list);
}

It is also reached from the error path of __register_pernet_operations().
It is reached from the setup_net() failure path through ops_undo_list() as
well, and there check_net() is still true for the net being torn down.

The two users later in this series seem to cope with this by ignoring the
list and filtering on !check_net(). Could the commit message be updated to
describe the scope the hook actually has?

Separately, the comment block above struct pernet_operations describes
how pre_exit(), exit() and exit_batch() are ordered and where
synchronize_rcu() falls between them. It is not updated for the new
member.

Would it make sense to document the following there?

  - pre_exit_batch() runs after all pre_exit() calls for the same ops.
  - It runs before the synchronize_rcu() that comes ahead of exit_rtnl(),
    exit() and exit_batch().
  - It runs while the netns devices may still be registered and up.
  - net_exit_list may contain live namespaces.

There is a related and more speculative case later in the series. The
change "net: Track state in ops_undo_list()" marks nets as
NET_PRE_EXIT_DONE whenever ops_list == &pernet_list. That includes the
setup_net() out_undo path, where check_net(net) is true.

In that case, rt_flush_dev(NULL) and rt6_uncached_list_flush_dev(NULL)
would skip that net's routes because check_net() is true. The per-device
flush on NETDEV_UNREGISTER would then return early because
net_pre_exit_done() is true.

If an uncached route pointing at a device of the failing netns existed at
that point, would its device reference ever be released? It is not clear
that such a route can exist in a netns that is not yet visible, so this
may not be reachable.

>  }
>  
>  static void ops_exit_rtnl_list(const struct list_head *ops_list,

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