[PATCH net] net: psp: check for device unregister when creating assoc

[PATCH net] net: psp: check for device unregister when creating assoc

[Jakub Kicinski]

psp_assoc_device_get_locked() obtains a psp_dev reference via
psp_dev_get_for_sock() (which uses psp_dev_tryget() under RCU);
it then acquires psd->lock and drops the reference. Before
the lock is taken, psp_dev_unregister() can run to completion:
take psd->lock, clear out state, unlock, drop the registration
reference.

The expectation is that the lock prevents device unregistration,
but much like with netdevs special care has to be taken when
"upgrading" a reference to a locked device. Add the missing
check if device is still alive. psp_dev_is_registered() exists
already but had no callers, which makes me wonder if I either
forgot to add this or lost the check during refactoring...

Reported-by: Yiming Qian <yimingqian591@gmail.com>
Fixes: 6b46ca260e22 ("net: psp: add socket security association code")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: daniel.zahka@gmail.com
CC: willemdebruijn.kernel@gmail.com
---
 net/psp/psp_nl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
index 6afd7707ec12..c7c377a7790d 100644
--- a/net/psp/psp_nl.c
+++ b/net/psp/psp_nl.c
@@ -320,6 +320,11 @@ int psp_assoc_device_get_locked(const struct genl_split_ops *ops,
 	id = info->attrs[PSP_A_ASSOC_DEV_ID];
 	if (psd) {
 		mutex_lock(&psd->lock);
+		if (!psp_dev_is_registered(psd)) {
+			mutex_unlock(&psd->lock);
+			err = -ENODEV;
+			goto err_psd_put;
+		}
 		if (id && psd->id != nla_get_u32(id)) {
 			mutex_unlock(&psd->lock);
 			NL_SET_ERR_MSG_ATTR(info->extack, id,
-- 
2.53.0

Re: [PATCH net] net: psp: check for device unregister when creating assoc

[Willem de Bruijn]

Jakub Kicinski wrote:
> psp_assoc_device_get_locked() obtains a psp_dev reference via
> psp_dev_get_for_sock() (which uses psp_dev_tryget() under RCU);
> it then acquires psd->lock and drops the reference. Before
> the lock is taken, psp_dev_unregister() can run to completion:
> take psd->lock, clear out state, unlock, drop the registration
> reference.
> 
> The expectation is that the lock prevents device unregistration,
> but much like with netdevs special care has to be taken when
> "upgrading" a reference to a locked device. Add the missing
> check if device is still alive. psp_dev_is_registered() exists
> already but had no callers, which makes me wonder if I either
> forgot to add this or lost the check during refactoring...
> 
> Reported-by: Yiming Qian <yimingqian591@gmail.com>
> Fixes: 6b46ca260e22 ("net: psp: add socket security association code")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
> CC: daniel.zahka@gmail.com
> CC: willemdebruijn.kernel@gmail.com
> ---
>  net/psp/psp_nl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> index 6afd7707ec12..c7c377a7790d 100644
> --- a/net/psp/psp_nl.c
> +++ b/net/psp/psp_nl.c
> @@ -320,6 +320,11 @@ int psp_assoc_device_get_locked(const struct genl_split_ops *ops,
>  	id = info->attrs[PSP_A_ASSOC_DEV_ID];
>  	if (psd) {
>  		mutex_lock(&psd->lock);
> +		if (!psp_dev_is_registered(psd)) {
> +			mutex_unlock(&psd->lock);
> +			err = -ENODEV;
> +			goto err_psd_put;
> +		}

This ensures that psd is valid for this caller of psp_dev_get_for_sock
and psp_dev_tryget, which is its only one (for now).

But is it confusing that psd can be cleared out while a reference is
held? Is the assumption that psp_dev_unregister usually holds the last
reference and its psp_dev_put will complete the clean up by calling
psp_dev_free. If so, would it make sense to defer everything
to psp_dev_free?

This is a simpler changes and fixes the issue for the only caller, so
LGTM. Just curious.

>  		if (id && psd->id != nla_get_u32(id)) {
>  			mutex_unlock(&psd->lock);
>  			NL_SET_ERR_MSG_ATTR(info->extack, id,
> -- 
> 2.53.0
> 

Re: [PATCH net] net: psp: check for device unregister when creating assoc

[Jakub Kicinski]

On Tue, 24 Mar 2026 18:40:00 -0400 Willem de Bruijn wrote:
> > @@ -320,6 +320,11 @@ int psp_assoc_device_get_locked(const struct genl_split_ops *ops,
> >  	id = info->attrs[PSP_A_ASSOC_DEV_ID];
> >  	if (psd) {
> >  		mutex_lock(&psd->lock);
> > +		if (!psp_dev_is_registered(psd)) {
> > +			mutex_unlock(&psd->lock);
> > +			err = -ENODEV;
> > +			goto err_psd_put;
> > +		}  
> 
> This ensures that psd is valid for this caller of psp_dev_get_for_sock
> and psp_dev_tryget, which is its only one (for now).
> 
> But is it confusing that psd can be cleared out while a reference is
> held? Is the assumption that psp_dev_unregister usually holds the last
> reference and its psp_dev_put will complete the clean up by calling
> psp_dev_free. If so, would it make sense to defer everything
> to psp_dev_free?
> 
> This is a simpler changes and fixes the issue for the only caller, so
> LGTM. Just curious.

Meaning we should add a dance around removal like netdev does?
Sleep until all the references are gone?
Subjectively I think it's ugly, and in netdev it didn't really
prevent races. Dunno, definitely subjective.

Re: [PATCH net] net: psp: check for device unregister when creating assoc

[Willem de Bruijn]

Jakub Kicinski wrote:
> On Tue, 24 Mar 2026 18:40:00 -0400 Willem de Bruijn wrote:
> > > @@ -320,6 +320,11 @@ int psp_assoc_device_get_locked(const struct genl_split_ops *ops,
> > >  	id = info->attrs[PSP_A_ASSOC_DEV_ID];
> > >  	if (psd) {
> > >  		mutex_lock(&psd->lock);
> > > +		if (!psp_dev_is_registered(psd)) {
> > > +			mutex_unlock(&psd->lock);
> > > +			err = -ENODEV;
> > > +			goto err_psd_put;
> > > +		}  
> > 
> > This ensures that psd is valid for this caller of psp_dev_get_for_sock
> > and psp_dev_tryget, which is its only one (for now).
> > 
> > But is it confusing that psd can be cleared out while a reference is
> > held? Is the assumption that psp_dev_unregister usually holds the last
> > reference and its psp_dev_put will complete the clean up by calling
> > psp_dev_free. If so, would it make sense to defer everything
> > to psp_dev_free?
> > 
> > This is a simpler changes and fixes the issue for the only caller, so
> > LGTM. Just curious.
> 
> Meaning we should add a dance around removal like netdev does?
> Sleep until all the references are gone?
> Subjectively I think it's ugly, and in netdev it didn't really
> prevent races. Dunno, definitely subjective.

Yeah, if any caller of psp_dev_unregister has to know that the device
is gone to proceed, it would require something like that. Okay, I see
that that is needlessly messy.