Re: [PATCH v7 net-next 1/5] psp: add admin/non-admin version of psp_device_get_locked

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
psp: add admin/non-admin version of psp_device_get_locked

This commit introduces two versions of psp_device_get_locked to distinguish
between admin operations (dev-set, key-rotation) and non-admin operations
(dev-get, rx-assoc, tx-assoc, get-stats).

> diff --git a/Documentation/netlink/specs/psp.yaml b/Documentation/netlink/specs/psp.yaml
> index f3a57782d2cf..fe2cdc966604 100644
> --- a/Documentation/netlink/specs/psp.yaml
> +++ b/Documentation/netlink/specs/psp.yaml

[ ... ]

> diff --git a/net/psp/psp-nl-gen.h b/net/psp/psp-nl-gen.h
> index 599c5f1c82f2..977355455395 100644
> --- a/net/psp/psp-nl-gen.h
> +++ b/net/psp/psp-nl-gen.h

[ ... ]

> diff --git a/net/psp/psp.h b/net/psp/psp.h
> index 9f19137593a0..0f9c4e4e52cb 100644
> --- a/net/psp/psp.h
> +++ b/net/psp/psp.h

[ ... ]

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 9508b6c38003..82de78a1d6bd 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c

[ ... ]

> diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> index 6afd7707ec12..b988f35412df 100644
> --- a/net/psp/psp_nl.c
> +++ b/net/psp/psp_nl.c
> @@ -41,7 +41,8 @@ static int psp_nl_reply_send(struct sk_buff *rsp, struct genl_info *info)
>  /* Device stuff */
>
>  static struct psp_dev *
> -psp_device_get_and_lock(struct net *net, struct nlattr *dev_id)
> +psp_device_get_and_lock(struct net *net, struct nlattr *dev_id,
> +			bool admin)
>  {
>  	struct psp_dev *psd;
>  	int err;
> @@ -56,7 +57,7 @@ psp_device_get_and_lock(struct net *net, struct nlattr *dev_id)
>  	mutex_lock(&psd->lock);
>  	mutex_unlock(&psp_devs_lock);
>
> -	err = psp_dev_check_access(psd, net);
> +	err = psp_dev_check_access(psd, net, admin);
>  	if (err) {
>  		mutex_unlock(&psd->lock);
>  		return ERR_PTR(err);
> @@ -65,6 +66,18 @@ psp_device_get_and_lock(struct net *net, struct nlattr *dev_id)
>  	return psd;
>  }
>
> +int psp_device_get_locked_admin(const struct genl_split_ops *ops,
> +				struct sk_buff *skb, struct genl_info *info)
> +{
> +	if (GENL_REQ_ATTR_CHECK(info, PSP_A_DEV_ID))
> +		return -EINVAL;
> +
> +	info->user_ptr[0] = psp_device_get_and_lock(genl_info_net(info),
> +						    info->attrs[PSP_A_DEV_ID],
> +						    true);
> +	return PTR_ERR_OR_ZERO(info->user_ptr[0]);
> +}
> +
>  int psp_device_get_locked(const struct genl_split_ops *ops,
>  			  struct sk_buff *skb, struct genl_info *info)
>  {
> @@ -72,7 +85,8 @@ int psp_device_get_locked(const struct genl_split_ops *ops,
>  		return -EINVAL;
>
>  	info->user_ptr[0] = psp_device_get_and_lock(genl_info_net(info),
> -						    info->attrs[PSP_A_DEV_ID]);
> +						    info->attrs[PSP_A_DEV_ID],
> +						    false);
>  	return PTR_ERR_OR_ZERO(info->user_ptr[0]);
>  }

This isn't a bug, but psp_device_get_locked_admin() and
psp_device_get_locked() are identical except for the true/false parameter
passed to psp_device_get_and_lock(). Would it be cleaner to introduce
a common helper like __psp_device_get_locked(bool admin) to avoid the
duplication?

Reference: https://lore.kernel.org/netdev/6fde8fe3-1ac9-4a47-89f5-b6f759c2f103@gmail.com/
Daniel Zahka raised this refactoring suggestion in v2, noting the two
functions are entirely identical modulo the admin parameter.

> @@ -160,7 +174,7 @@ static int
>  psp_nl_dev_get_dumpit_one(struct sk_buff *rsp, struct netlink_callback *cb,
>  			  struct psp_dev *psd)
>  {
> -	if (psp_dev_check_access(psd, sock_net(rsp->sk)))
> +	if (psp_dev_check_access(psd, sock_net(rsp->sk), false))
>  		return 0;
>
>  	return psp_nl_dev_fill(psd, rsp, genl_info_dump(cb));

[ ... ]