The Linux Kernel Mailing List
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Kiran Kella <kiran.kella@broadcom.com>
Cc: daniel.zahka@gmail.com, willemdebruijn.kernel@gmail.com,
	davem@davemloft.net, edumazet@google.com, pabeni@redhat.com,
	horms@kernel.org, weibunny@fb.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	jayakrishnan.udayavarma@broadcom.com, ajit.khaparde@broadcom.com,
	akhilesh.samineni@broadcom.com,
	Vikas Gupta <vikas.gupta@broadcom.com>,
	Bhargava Marreddy <bhargava.marreddy@broadcom.com>,
	Daniel Zahka <daniel.zahka@broadcom.com>
Subject: Re: [PATCH net v2] psp: fix NULL genl_sock deref race with concurrent netns teardown
Date: Tue, 21 Jul 2026 07:52:52 -0700	[thread overview]
Message-ID: <20260721075252.1d577783@kernel.org> (raw)
In-Reply-To: <20260707185937.3177211-1-kiran.kella@broadcom.com>

On Tue,  7 Jul 2026 11:59:37 -0700 Kiran Kella wrote:
> The race occurs between network namespace removal and PSP device
> unregistration.  When a netns is deleted while a PSP device associated
> with that netns is concurrently being removed, psp_dev_unregister()
> triggers psp_nl_notify_dev() to send a device change notification.
> Concurrently, cleanup_net() running in the netns workqueue calls
> genl_pernet_exit(), which sets net->genl_sock to NULL. If
> genl_pernet_exit() wins the race, two sites in psp_nl_multicast_per_ns()
> then dereference the NULL socket and crash:
> 
> CPU 0 (netns teardown)       CPU 1 (PSP device unregister)
> ======================       =============================
> cleanup_net [workqueue]
>   genl_pernet_exit()         psp_dev_unregister()
>     net->genl_sock = NULL      psp_nl_notify_dev()
>                                  psp_nl_multicast_per_ns()
>                                    build_ntf()
>                                      -> netlink_has_listeners(NULL)  
>                                      /* crash */
>                                    genlmsg_multicast_netns()
>                                      -> nlmsg_multicast_filtered(NULL)  
>                                      /* crash */

Can you explain how you hit this, in reality? Seems like an extreme
corner case to me.

> Both the main_net path (derived from psd->main_netdev) and each
> assoc_net entry in psd->assoc_dev_list are affected.

I don't see how assoc_dev_list is affected. Assoc dev list has 
a notifier which should serialize the netdev unreg on psd->lock. 
And netdev unreg must happen before the netns dismantle.

> Fix by replacing the bare dev_net() calls with maybe_get_net().
> maybe_get_net() returns NULL if the namespace is already dying.
> Holding the reference ensures genl_sock remains valid across both the
> build_ntf() and genlmsg_multicast_netns() calls.
> 
> Fixes: 00c94ca2b99e ("psp: base PSP device support")
> Fixes: 06c2dce2d0f6 ("psp: add new netlink cmd for dev-assoc and dev-disassoc")
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Vikas Gupta <vikas.gupta@broadcom.com>
> Reviewed-by: Bhargava Marreddy <bhargava.marreddy@broadcom.com>
> Reviewed-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
> Reviewed-by: Daniel Zahka <daniel.zahka@broadcom.com>
> Tested-by: Daniel Zahka <daniel.zahka@broadcom.com>
> Signed-off-by: Kiran Kella <kiran.kella@broadcom.com>
> ---
> v2:
>  - get rid of the extra struct net *net, by doing
> (!maybe_get_net(assoc_net)) directly (as suggested by Daniel Zahka)
> 
> v1: https://lore.kernel.org/all/20260703112431.2860506-1-kiran.kella@broadcom.com/
> 
>  net/psp/psp_nl.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> index 9610d8c456ff..1438dbb07949 100644
> --- a/net/psp/psp_nl.c
> +++ b/net/psp/psp_nl.c
> @@ -62,7 +62,10 @@ psp_nl_multicast_per_ns(struct psp_dev *psd, unsigned int group,
>  	struct net *main_net;
>  	struct sk_buff *ntf;
>  
> -	main_net = dev_net(psd->main_netdev);

comment like:

	/* device may be changing netns in parallel */

> +	main_net = maybe_get_net(dev_net(psd->main_netdev));

not sure how the dev_net() is safe here if we're not holding any
relevant lock. Do we need rcu?

> +	if (!main_net)
> +		return;
> +
>  	xa_init(&sent_nets);
>  
>  	list_for_each_entry(entry, &psd->assoc_dev_list, dev_list) {
> @@ -77,21 +80,23 @@ psp_nl_multicast_per_ns(struct psp_dev *psd, unsigned int group,
>  		if (ret == -EBUSY)
>  			continue;
>  
> -		ntf = build_ntf(psd, assoc_net, ctx);
> -		if (!ntf)
> +		if (!maybe_get_net(assoc_net))
>  			continue;

AFAICT we clear the psd on netns move for assoc dev so I don't think
this is needed today.

> -		genlmsg_multicast_netns(&psp_nl_family, assoc_net, ntf, 0,
> -					group, GFP_KERNEL);
> +		ntf = build_ntf(psd, assoc_net, ctx);
> +		if (ntf)
> +			genlmsg_multicast_netns(&psp_nl_family, assoc_net, ntf,
> +						0, group, GFP_KERNEL);
> +		put_net(assoc_net);
>  	}
>  	xa_destroy(&sent_nets);
>  
>  	/* Send to main device netns */
>  	ntf = build_ntf(psd, main_net, ctx);
> -	if (!ntf)
> -		return;
> -	genlmsg_multicast_netns(&psp_nl_family, main_net, ntf, 0, group,
> -				GFP_KERNEL);
> +	if (ntf)
> +		genlmsg_multicast_netns(&psp_nl_family, main_net, ntf, 0, group,
> +					GFP_KERNEL);
> +	put_net(main_net);
>  }
>  
>  static struct sk_buff *psp_nl_clone_ntf(struct psp_dev *psd, struct net *net,


  parent reply	other threads:[~2026-07-21 14:52 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-07 18:59 [PATCH net v2] psp: fix NULL genl_sock deref race with concurrent netns teardown Kiran Kella
2026-07-07 19:50 ` Wei Wang
2026-07-21 14:52 ` Jakub Kicinski [this message]
2026-07-23 15:44   ` Kiran Kella

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260721075252.1d577783@kernel.org \
    --to=kuba@kernel.org \
    --cc=ajit.khaparde@broadcom.com \
    --cc=akhilesh.samineni@broadcom.com \
    --cc=bhargava.marreddy@broadcom.com \
    --cc=daniel.zahka@broadcom.com \
    --cc=daniel.zahka@gmail.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=jayakrishnan.udayavarma@broadcom.com \
    --cc=kiran.kella@broadcom.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=vikas.gupta@broadcom.com \
    --cc=weibunny@fb.com \
    --cc=willemdebruijn.kernel@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox