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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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
>