Re: [PATCH net-next v1 04/15] psp: add support for socket-less psp associations

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

On Tue Nov 10, 2026 at 7:55 PM EST, Todd Freed wrote:
> currently, a psp association is bound to a tcp socket when it is
> created, and can hold both an rx and a tx key.
>
> add a small registry of un-bound associations. These have either an rx
> or tx flavor, and are referenced by an id. other parts of the stack can
> then attach them to packets. see the subsequent patches, where the tc
> psp action does just this.
>
> an rx association records the spi, version, and device key generation,
> primarily for describe (assoc-get). a tc filter matches these fields
> with the flower keys psp_spi, psp_ver, and psp_dev instead of
> referencing the association by id, to facilitate hw offload. these
> matches are added in a subsequent patch.
>
> adds the following new netlink commands for managing these associations.
> note, these are admin-only and restricted to the netns of the psp device
>
>   rx-spi-alloc  allocate an rx spi + derived key, and publish an rx
>                 association for it, returns its id. the key is
>                 delivered to the peer out of band
>   assoc-add     create a tx association and install a tx key, returns
>                 its id
>   assoc-del     drop the registry's reference to an association
>   assoc-get     get / dump associations ; only the spi is reported
>
> assoc-get and assoc-del operate on both rx and tx associations.
>
> socket associations do not go into the registry, and these apis do not
> address them. so for example, assoc-get does not list them.
>
> socket-less associations are on the device's association lists, like
> socket associations. they are therefore marked stale by key rotation in
> the same way, except tx ones: they hold the peer's key, which the
> rotation of the device key doesn't affect. assoc-get reports it
> (stale), so that user space can identify such entries.
>
> key material enters through netlink and is never reported back. no key
> is stored for rx associations, and users reference associations by id
> and never handle keys
>
> published associations are dropped from the registry when their psp
> device is unregistered.
>

I think the design could be improved by not reusing struct psp_assoc for
the socketless rx spi accounting, and by not placing the socketless tx
assoc on the active, prev, and stale lists next to the socket
associations. I'll try to comment inline to give you a better idea of
what I'm talking about.

In general, what I'd like to minimize is having one object used for
three different things, where each usecase has its own subset of fields
active, and then having a bunch of glue in places like
psp_assocs_key_rotated(), where you have to check if the assoc is
socketless, followed by if it is tx or rx.

[...]

> +int psp_nl_rx_spi_alloc_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct psp_dev *psd = info->user_ptr[0];
> +	struct psp_key_parsed key;
> +	struct psp_assoc *pas;
> +	struct sk_buff *rsp;
> +	u32 version;
> +	int err;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, PSP_A_ASSOC_VERSION))
> +		return -EINVAL;
> +
> +	version = nla_get_u32(info->attrs[PSP_A_ASSOC_VERSION]);
> +	if (!(psd->caps->versions & (1 << version))) {
> +		NL_SET_BAD_ATTR(info->extack, info->attrs[PSP_A_ASSOC_VERSION]);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	rsp = psp_nl_reply_new(info);
> +	if (!rsp)
> +		return -ENOMEM;
> +
> +	err = psd->ops->rx_spi_alloc(psd, version, &key, info->extack);
> +	if (err)
> +		goto err_free_rsp;
> +
> +	pas = psp_assoc_create(psd);

I'm not sure the kernel actually needs any bookkeeping at all for these
generic rx spi allocations. You could probably just pass the spi, dev
id, key, and probably rotation count (as a device key generation of
sorts) to the user without creating a kernel object. For socket
psp_assocs, the rx state is consulted on the datapath before queueing to
the socket. For tx state in all psp_assocs, there is the possibility
that an SADB handle needs to be cleaned up. Neither of those apply here
or in the rest of the series as I understand it.

I figure the tc ingress rule can just take the spi and dev id by value
rather than needing a reference to an object that we create here.

> +	if (!pas) {
> +		err = -ENOMEM;
> +		goto err_free_rsp;
> +	}
> +	pas->version = version;
> +	pas->rx.spi = key.spi;
> +
> +	err = psp_assoc_publish(pas);
> +	if (err)
> +		goto err_put_pas;
> +
> +	if (nla_put_u32(rsp, PSP_A_ASSOC_ID, pas->id) ||
> +	    nla_put_u32(rsp, PSP_A_ASSOC_DEV_ID, psd->id) ||
> +	    psp_nl_put_key(rsp, PSP_A_ASSOC_RX_KEY, version, &key)) {
> +		psp_assoc_unpublish(pas);
> +		err = -EMSGSIZE;
> +		goto err_put_pas;
> +	}
> +
> +	psp_assoc_put(pas);
> +	memzero_explicit(&key, sizeof(key));
> +
> +	return psp_nl_reply_send(rsp, info);
> +
> +err_put_pas:
> +	psp_assoc_put(pas);
> +err_free_rsp:
> +	memzero_explicit(&key, sizeof(key));
> +	nlmsg_free(rsp);
> +	return err;
> +}
> +

[...]

> @@ -310,7 +311,13 @@ void psp_assocs_key_rotated(struct psp_dev *psd)
>  	 * be able to Rx any traffic.
>  	 */
>  	list_for_each_entry_safe(pas, next, &psd->prev_assocs, assocs_list) {
> -		pas->generation |= ~PSP_GEN_VALID_MASK;
> +		/* A socket-less Tx association holds the peer's key, which
> +		 * the rotation of the device key doesn't affect
> +		 */
> +		if (pas->id && psp_assoc_is_tx(pas))
> +			continue;
> +		WRITE_ONCE(pas->generation,
> +			   pas->generation | ~PSP_GEN_VALID_MASK);
>  		psd->stats.stales++;
>  	}
>  	list_splice_init(&psd->prev_assocs, &psd->stale_assocs);

I would create a separate list for the socketless tx assocs, so that
SADB handles can be flushed on psp_dev_unregister(). Then you can just
leave this function untouched, because invalidating the generation is
only for __psp_sk_rx_policy_check(), which isn't used by the socketless
rx assocs.