Re: [PATCH v10 net-next 2/5] psp: add new netlink cmd for dev-assoc and dev-disassoc

[Daniel Zahka <daniel.zahka@gmail.com>]

On 4/5/26 1:58 AM, Wei Wang wrote:
> From: Wei Wang <weibunny@fb.com>
>
> The main purpose of this cmd is to be able to associate a
> non-psp-capable device (e.g. veth or netkit) with a psp device.
> One use case is if we create a pair of veth/netkit, and assign 1 end
> inside a netns, while leaving the other end within the default netns,
> with a real PSP device, e.g. netdevsim or a physical PSP-capable NIC.
> With this command, we could associate the veth/netkit inside the netns
> with PSP device, so the virtual device could act as PSP-capable device
> to initiate PSP connections, and performs PSP encryption/decryption on
> the real PSP device.
>
> Signed-off-by: Wei Wang <weibunny@fb.com>
> ---
>   Documentation/netlink/specs/psp.yaml |  67 +++++-
>   include/net/psp/types.h              |  15 ++
>   include/uapi/linux/psp.h             |  13 ++
>   net/psp/psp-nl-gen.c                 |  32 +++
>   net/psp/psp-nl-gen.h                 |   2 +
>   net/psp/psp_main.c                   |  20 ++
>   net/psp/psp_nl.c                     | 319 ++++++++++++++++++++++++++-
>   7 files changed, 457 insertions(+), 11 deletions(-)
>
...
>   
> +/**
> + * Admin version of psp_device_get_locked() where it returns psd only if
> + * current netns is the same as psd->main_netdev's netns.
> + */
>   int psp_device_get_locked_admin(const struct genl_split_ops *ops,
>   				struct sk_buff *skb, struct genl_info *info)
>   {
>   	return __psp_device_get_locked(ops, skb, info, true);
>   }
>   
> +/**
> + * Non-admin version of psp_device_get_locked() where it returns psd in netns
> + * for not only psd->main_netdev but all netdevs in psd->assoc_dev_list.
> + */
>   int psp_device_get_locked(const struct genl_split_ops *ops,
>   			  struct sk_buff *skb, struct genl_info *info)
>   {
> @@ -103,11 +179,74 @@ psp_device_unlock(const struct genl_split_ops *ops, struct sk_buff *skb,
>   		sockfd_put(socket);
>   }


There's a warning that these comments have the kdoc open sequence, but 
are not proper kdoc comments.

> +
>   static int
>   psp_nl_dev_fill(struct psp_dev *psd, struct sk_buff *rsp,
>   		const struct genl_info *info)
>   {
> +	struct net *cur_net;
>   	void *hdr;
> +	int err;
> +
> +	cur_net = genl_info_net(info);
> +
> +	/* Skip this device if we're in an associated netns but have no
> +	 * associated devices in cur_net
> +	 */
> +	if (cur_net != dev_net(psd->main_netdev) &&
> +	    !psp_has_assoc_dev_in_ns(psd, cur_net))
> +		return 0;
>   


Is this branch dead code given we either arrived here via 
psp_dev_check_access(), or psp_nl_build_dev_ntf() which should only use 
associated netns's?

>   
> +int psp_nl_dev_assoc_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct psp_dev *psd = info->user_ptr[0];
> +	struct psp_assoc_dev *psp_assoc_dev;
> +	struct net_device *assoc_dev;
> +	struct sk_buff *rsp;
> +	u32 assoc_ifindex;
> +	struct net *net;
> +	int nsid;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, PSP_A_DEV_IFINDEX))
> +		return -EINVAL;
> +
> +	if (info->attrs[PSP_A_DEV_NSID]) {
> +		nsid = nla_get_s32(info->attrs[PSP_A_DEV_NSID]);
> +
> +		net = get_net_ns_by_id(genl_info_net(info), nsid);
> +		if (!net) {
> +			NL_SET_BAD_ATTR(info->extack,
> +					info->attrs[PSP_A_DEV_NSID]);
> +			return -EINVAL;
> +		}
> +	} else {
> +		net = get_net(genl_info_net(info));
> +	}
> +
> +	psp_assoc_dev = kzalloc(sizeof(*psp_assoc_dev), GFP_KERNEL);
> +	if (!psp_assoc_dev) {
> +		put_net(net);
> +		return -ENOMEM;
> +	}
> +
> +	assoc_ifindex = nla_get_u32(info->attrs[PSP_A_DEV_IFINDEX]);
> +	assoc_dev = netdev_get_by_index(net, assoc_ifindex,
> +					&psp_assoc_dev->dev_tracker,
> +					GFP_KERNEL);
> +	if (!assoc_dev) {
> +		put_net(net);
> +		kfree(psp_assoc_dev);
> +		NL_SET_BAD_ATTR(info->extack, info->attrs[PSP_A_DEV_IFINDEX]);
> +		return -ENODEV;
> +	}
> +
> +	/* Check if device is already associated with a PSP device */
> +	if (cmpxchg(&assoc_dev->psp_dev, NULL, RCU_INITIALIZER(psd))) {
> +		NL_SET_ERR_MSG(info->extack,
> +			       "Device already associated with a PSP device");
> +		netdev_put(assoc_dev, &psp_assoc_dev->dev_tracker);
> +		put_net(net);
> +		kfree(psp_assoc_dev);
> +		return -EBUSY;
> +	}
> +
> +	psp_assoc_dev->assoc_dev = assoc_dev;
> +	rsp = psp_nl_reply_new(info);
> +	if (!rsp) {
> +		rcu_assign_pointer(assoc_dev->psp_dev, NULL);
> +		netdev_put(assoc_dev, &psp_assoc_dev->dev_tracker);
> +		put_net(net);
> +		kfree(psp_assoc_dev);
> +		return -ENOMEM;
> +	}
> +
> +	list_add_tail(&psp_assoc_dev->dev_list, &psd->assoc_dev_list);
> +
> +	put_net(net);
> +
> +	psp_nl_notify_dev(psd, PSP_CMD_DEV_CHANGE_NTF);
> +
> +	return psp_nl_reply_send(rsp, info);
> +}
> +


This function could probably benefit from a goto style cleanup chain, 
given the overlapping set of actions to unwind at each error.

>   
>   int psp_assoc_device_get_locked(const struct genl_split_ops *ops,
> @@ -320,7 +617,9 @@ int psp_assoc_device_get_locked(const struct genl_split_ops *ops,
>   
>   	psd = psp_dev_get_for_sock(socket->sk);
>   	if (psd) {
> +		mutex_lock(&psd->lock);
>   		err = psp_dev_check_access(psd, genl_info_net(info), false);
> +		mutex_unlock(&psd->lock);


This looks like a "TOCTOU" issue on the mutable assoc_dev_list, but I 
think it ends up being a benign race.


>   		if (err) {
>   			psp_dev_put(psd);
>   			psd = NULL;


Some minor comments, but otherwise:

Reviewed-by: Daniel Zahka <daniel.zahka@gmail.com>