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

[Paolo Abeni <pabeni@redhat.com>]

On 4/9/26 1:14 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>
> Reviewed-by: Daniel Zahka <daniel.zahka@gmail.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                     | 325 ++++++++++++++++++++++++++-
>  7 files changed, 462 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/netlink/specs/psp.yaml b/Documentation/netlink/specs/psp.yaml
> index c54e1202cbe0..3d1b7223e084 100644
> --- a/Documentation/netlink/specs/psp.yaml
> +++ b/Documentation/netlink/specs/psp.yaml
> @@ -13,6 +13,17 @@ definitions:
>                hdr0-aes-gmac-128, hdr0-aes-gmac-256]
>  
>  attribute-sets:
> +  -
> +    name: assoc-dev-info
> +    attributes:
> +      -
> +        name: ifindex
> +        doc: ifindex of an associated network device.
> +        type: u32
> +      -
> +        name: nsid
> +        doc: Network namespace ID of the associated device.
> +        type: s32
>    -
>      name: dev
>      attributes:
> @@ -24,7 +35,9 @@ attribute-sets:
>            min: 1
>        -
>          name: ifindex
> -        doc: ifindex of the main netdevice linked to the PSP device.
> +        doc: |
> +          ifindex of the main netdevice linked to the PSP device,
> +          or the ifindex to associate with the PSP device.
>          type: u32
>        -
>          name: psp-versions-cap
> @@ -38,6 +51,28 @@ attribute-sets:
>          type: u32
>          enum: version
>          enum-as-flags: true
> +      -
> +        name: assoc-list
> +        doc: List of associated virtual devices.
> +        type: nest
> +        nested-attributes: assoc-dev-info
> +        multi-attr: true
> +      -
> +        name: nsid
> +        doc: |
> +          Network namespace ID for the device to associate/disassociate.
> +          Optional for dev-assoc and dev-disassoc; if not present, the
> +          device is looked up in the caller's network namespace.
> +        type: s32
> +      -
> +        name: by-association
> +        doc: |
> +          Flag indicating the PSP device is an associated device from a
> +          different network namespace.
> +          Present when in associated namespace, absent when in primary/host
> +          namespace.
> +        type: flag
> +
>    -
>      name: assoc
>      attributes:
> @@ -170,6 +205,8 @@ operations:
>              - ifindex
>              - psp-versions-cap
>              - psp-versions-ena
> +            - assoc-list
> +            - by-association
>          pre: psp-device-get-locked
>          post: psp-device-unlock
>        dump:
> @@ -279,6 +316,34 @@ operations:
>          post: psp-device-unlock
>        dump:
>          reply: *stats-all
> +    -
> +      name: dev-assoc
> +      doc: Associate a network device with a PSP device.
> +      attribute-set: dev
> +      do:
> +        request:
> +          attributes:
> +            - id
> +            - ifindex
> +            - nsid
> +        reply:
> +          attributes: []
> +        pre: psp-device-get-locked
> +        post: psp-device-unlock
> +    -
> +      name: dev-disassoc
> +      doc: Disassociate a network device from a PSP device.
> +      attribute-set: dev
> +      do:
> +        request:
> +          attributes:
> +            - id
> +            - ifindex
> +            - nsid
> +        reply:
> +          attributes: []
> +        pre: psp-device-get-locked
> +        post: psp-device-unlock
>  
>  mcast-groups:
>    list:
> diff --git a/include/net/psp/types.h b/include/net/psp/types.h
> index 25a9096d4e7d..4bd432ed107a 100644
> --- a/include/net/psp/types.h
> +++ b/include/net/psp/types.h
> @@ -5,6 +5,7 @@
>  
>  #include <linux/mutex.h>
>  #include <linux/refcount.h>
> +#include <net/net_trackers.h>
>  
>  struct netlink_ext_ack;
>  
> @@ -43,9 +44,22 @@ struct psp_dev_config {
>  	u32 versions;
>  };
>  
> +/**
> + * struct psp_assoc_dev - wrapper for associated net_device
> + * @dev_list: list node for psp_dev::assoc_dev_list
> + * @assoc_dev: the associated net_device
> + * @dev_tracker: tracker for the net_device reference
> + */
> +struct psp_assoc_dev {
> +	struct list_head dev_list;
> +	struct net_device *assoc_dev;
> +	netdevice_tracker dev_tracker;
> +};
> +
>  /**
>   * struct psp_dev - PSP device struct
>   * @main_netdev: original netdevice of this PSP device
> + * @assoc_dev_list: list of psp_assoc_dev entries associated with this PSP device
>   * @ops:	driver callbacks
>   * @caps:	device capabilities
>   * @drv_priv:	driver priv pointer
> @@ -67,6 +81,7 @@ struct psp_dev_config {
>   */
>  struct psp_dev {
>  	struct net_device *main_netdev;
> +	struct list_head assoc_dev_list;
>  
>  	struct psp_dev_ops *ops;
>  	struct psp_dev_caps *caps;
> diff --git a/include/uapi/linux/psp.h b/include/uapi/linux/psp.h
> index a3a336488dc3..1c8899cd4da5 100644
> --- a/include/uapi/linux/psp.h
> +++ b/include/uapi/linux/psp.h
> @@ -17,11 +17,22 @@ enum psp_version {
>  	PSP_VERSION_HDR0_AES_GMAC_256,
>  };
>  
> +enum {
> +	PSP_A_ASSOC_DEV_INFO_IFINDEX = 1,
> +	PSP_A_ASSOC_DEV_INFO_NSID,
> +
> +	__PSP_A_ASSOC_DEV_INFO_MAX,
> +	PSP_A_ASSOC_DEV_INFO_MAX = (__PSP_A_ASSOC_DEV_INFO_MAX - 1)
> +};
> +
>  enum {
>  	PSP_A_DEV_ID = 1,
>  	PSP_A_DEV_IFINDEX,
>  	PSP_A_DEV_PSP_VERSIONS_CAP,
>  	PSP_A_DEV_PSP_VERSIONS_ENA,
> +	PSP_A_DEV_ASSOC_LIST,
> +	PSP_A_DEV_NSID,
> +	PSP_A_DEV_BY_ASSOCIATION,
>  
>  	__PSP_A_DEV_MAX,
>  	PSP_A_DEV_MAX = (__PSP_A_DEV_MAX - 1)
> @@ -74,6 +85,8 @@ enum {
>  	PSP_CMD_RX_ASSOC,
>  	PSP_CMD_TX_ASSOC,
>  	PSP_CMD_GET_STATS,
> +	PSP_CMD_DEV_ASSOC,
> +	PSP_CMD_DEV_DISASSOC,
>  
>  	__PSP_CMD_MAX,
>  	PSP_CMD_MAX = (__PSP_CMD_MAX - 1)
> diff --git a/net/psp/psp-nl-gen.c b/net/psp/psp-nl-gen.c
> index 1f5e73e7ccc1..114299c64423 100644
> --- a/net/psp/psp-nl-gen.c
> +++ b/net/psp/psp-nl-gen.c
> @@ -53,6 +53,20 @@ static const struct nla_policy psp_get_stats_nl_policy[PSP_A_STATS_DEV_ID + 1] =
>  	[PSP_A_STATS_DEV_ID] = NLA_POLICY_MIN(NLA_U32, 1),
>  };
>  
> +/* PSP_CMD_DEV_ASSOC - do */
> +static const struct nla_policy psp_dev_assoc_nl_policy[PSP_A_DEV_NSID + 1] = {
> +	[PSP_A_DEV_ID] = NLA_POLICY_MIN(NLA_U32, 1),
> +	[PSP_A_DEV_IFINDEX] = { .type = NLA_U32, },
> +	[PSP_A_DEV_NSID] = { .type = NLA_S32, },
> +};
> +
> +/* PSP_CMD_DEV_DISASSOC - do */
> +static const struct nla_policy psp_dev_disassoc_nl_policy[PSP_A_DEV_NSID + 1] = {
> +	[PSP_A_DEV_ID] = NLA_POLICY_MIN(NLA_U32, 1),
> +	[PSP_A_DEV_IFINDEX] = { .type = NLA_U32, },
> +	[PSP_A_DEV_NSID] = { .type = NLA_S32, },
> +};
> +
>  /* Ops table for psp */
>  static const struct genl_split_ops psp_nl_ops[] = {
>  	{
> @@ -119,6 +133,24 @@ static const struct genl_split_ops psp_nl_ops[] = {
>  		.dumpit	= psp_nl_get_stats_dumpit,
>  		.flags	= GENL_CMD_CAP_DUMP,
>  	},
> +	{
> +		.cmd		= PSP_CMD_DEV_ASSOC,
> +		.pre_doit	= psp_device_get_locked,
> +		.doit		= psp_nl_dev_assoc_doit,
> +		.post_doit	= psp_device_unlock,
> +		.policy		= psp_dev_assoc_nl_policy,
> +		.maxattr	= PSP_A_DEV_NSID,
> +		.flags		= GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd		= PSP_CMD_DEV_DISASSOC,
> +		.pre_doit	= psp_device_get_locked,
> +		.doit		= psp_nl_dev_disassoc_doit,
> +		.post_doit	= psp_device_unlock,
> +		.policy		= psp_dev_disassoc_nl_policy,
> +		.maxattr	= PSP_A_DEV_NSID,
> +		.flags		= GENL_CMD_CAP_DO,

Sashiko notes that the above allows deleteing an associations bypassing
the netns boundaries. Do you need ADMIN_PERM flag or exlicit checks in
the doit cb?

> @@ -292,6 +455,145 @@ int psp_nl_key_rotate_doit(struct sk_buff *skb, struct genl_info *info)
>  	return err;
>  }
>  
> +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, err;
> +
> +	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_nl_dev_disassoc_doit() has the same code; perhaps it would be worthy
move it in a common helper, called via pre_doit()? It should also
simplify the cleanup paths.

> +
> +	psp_assoc_dev = kzalloc(sizeof(*psp_assoc_dev), GFP_KERNEL);
> +	if (!psp_assoc_dev) {
> +		err = -ENOMEM;
> +		goto alloc_err;
> +	}
> +
> +	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) {
> +		NL_SET_BAD_ATTR(info->extack, info->attrs[PSP_A_DEV_IFINDEX]);
> +		err = -ENODEV;
> +		goto assoc_dev_err;
> +	}
> +
> +	/* 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");
> +		err = -EBUSY;
> +		goto cmpxchg_err;
> +	}
> +
> +	psp_assoc_dev->assoc_dev = assoc_dev;
> +	rsp = psp_nl_reply_new(info);
> +	if (!rsp) {
> +		err = -ENOMEM;
> +		goto rsp_err;
> +	}
> +
> +	list_add_tail(&psp_assoc_dev->dev_list, &psd->assoc_dev_list);

Sashiko says:

---
list_add_tail(&psp_assoc_dev->dev_list, &psd->assoc_dev_list);
There doesn't seem to be a limit on the number of devices that can be
associated with a single PSP device.
If a user repeatedly associates devices, could the generated netlink message
in psp_nl_dev_fill() exceed the maximum allowed size (GENLMSG_DEFAULT_SIZE),
causing it to fail with -EMSGSIZE and permanently break PSP_CMD_DEV_GET
and management notifications for the device?
--

/P