Re: [PATCH net] seg6: fix tun_src RCU lifetime on namespace teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] The patch adds a reachable NULL pointer dereference on the
  seg6 datapath during netns teardown.

--

seg6: fix tun_src RCU lifetime on namespace teardown

This patch adds a pre_exit callback to seg6 pernet ops. The callback
saves the per-netns tun_src pointer in a new tun_src_old field and then
clears the RCU pointer. The exit callback frees tun_src_old after the
grace period that runs between pre_exit and exit.

> diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
> index 8c2b156c227a7..670052290e43c 100644
> --- a/net/ipv6/seg6.c
> +++ b/net/ipv6/seg6.c
> @@ -447,18 +447,28 @@ static int __net_init seg6_net_init(struct net *net)
>  	return 0;
>  }
>  
> +static void __net_exit seg6_net_pre_exit(struct net *net)
> +{
> +	struct seg6_pernet_data *sdata = seg6_pernet(net);
> +
> +	/* Save tun_src for exit to free after the RCU grace period. */
> +	sdata->tun_src_old = rcu_dereference_raw(sdata->tun_src);
> +	RCU_INIT_POINTER(sdata->tun_src, NULL);

[Severity: Critical]
Can this cause a NULL pointer dereference in set_tun_src() while the
netns is being torn down?

set_tun_src() in net/ipv6/seg6_iptunnel.c assumes tun_src is never NULL.
If the route has no per-route tunsrc configured (the default), it does:

	rcu_read_lock();
	tun_src = rcu_dereference(sdata->tun_src);

	if (!ipv6_addr_any(tun_src)) {
		memcpy(saddr, tun_src, sizeof(struct in6_addr));

ipv6_addr_any() dereferences its argument in both config variants, so a
NULL tun_src faults there.

In ops_undo_list(), all pre_exit callbacks run first. After that comes
synchronize_rcu_expedited(), then ops_exit_rtnl_list(), then
ops_exit_list(). The netns devices are unregistered later, in
default_device_exit_batch() during the exit phase. That means the dying
netns keeps its registered devices and seg6 lwtunnel routes after
seg6_net_pre_exit() has published NULL.

A packet that arrives in that window, for example from a veth peer in a
live netns or from a physical NIC, would follow:

seg6_input_core() / seg6_output_core()
  -> seg6_do_srh()
    -> seg6_do_srh_encap() / seg6_do_srh_encap_red() / seg6_do_srh_inline()
      -> set_tun_src()
        -> ipv6_addr_any(NULL)

This runs in softirq or xmit context. The grace period does not close
the window, because it only waits for readers that already exist.
Readers that load the pointer after the clear still see NULL.

Before this patch, tun_src was never NULL between init and exit. Would
it be better to leave the pointer in place and keep the kfree() in
seg6_net_exit()? By the time exit runs, a grace period has already
passed.

seg6_genl_get_tunsrc() also passes the pointer to nla_put() without a
NULL check. That path is probably not reachable on a dying netns, since
a genl socket holds a netns reference.

> +}
> +
>  static void __net_exit seg6_net_exit(struct net *net)
>  {
>  	struct seg6_pernet_data *sdata = seg6_pernet(net);
>  
>  	seg6_hmac_net_exit(net);
>  
> -	kfree(rcu_dereference_raw(sdata->tun_src));
> +	kfree(sdata->tun_src_old);
>  	kfree(sdata);
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023195234.3524462-1-tristmd%40gmail.com