Re: [PATCH net-next v3 3/5] psp: defer tx key deletions for SADB drivers

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

On Sat Nov 7, 2026 at 3:59 PM EST, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] This patch removes the psp_dev_has_sadb() -> -EOPNOTSUPP guard
>   from psp_sock_tx_rekey() (net/psp/psp_sock.c) and replaces the old…
> - [Low] The patch adds an implicit rule for SADB drivers (those with
>   tx_key_add/tx_key_del) that is written down only in the commit…
>
> --
>
> psp: defer tx key deletions for SADB drivers
>
> This commit enables Tx rekey on devices that keep Tx keys in an SADB. A
> superseded Tx key is now deleted only after a BQL-based descriptor grace
> period has passed on every active Tx queue, and a delayed work polls the
> queued and completed byte counts to detect the end of that period.
> CONFIG_INET_PSP now depends on CONFIG_BQL, and SADB drivers must be ops
> locked.
>
>> diff --git a/Documentation/networking/psp.rst b/Documentation/networking/psp.rst
>> index 43f222b38d3d4..ceb1e232317fc 100644
>> --- a/Documentation/networking/psp.rst
>> +++ b/Documentation/networking/psp.rst
>> @@ -202,6 +202,48 @@ Drivers must use ``psp_skb_get_assoc_rcu()`` to check if PSP Tx offload
>
> [ ... ]
>
>> +Drivers which implement the Tx key callbacks must meet the following
>> +requirements:
>> +
>> +* The driver must be "ops locked"
>> +  (see Documentation/networking/netdevices.rst).
>
> [ ... ]
>
>> +* Before calling ``psp_dev_unregister()``, the driver must ensure that all
>> +  outstanding Tx descriptors containing key references have been completed
>> +  or discarded. PSP core deletes all remaining Tx keys during
>> +  unregistration without waiting for a grace period.
>> +
>
> [Severity: Low]
> Should this list also say that psp_dev_unregister() must not be called
> with the netdev instance lock held? Right now that rule is only in the
> commit message.
>
> psp_dev_unregister() now begins with psp_deferred_del_stop(), which waits
> for the work:
>
> psp_deferred_del_stop() {
> 	disable_delayed_work_sync(&psd->tx_del.work);
> }
>
> The work takes psd->lock first and then the instance lock:
>
> psp_deferred_del_work() {
> 	...
> 	mutex_lock(&psd->lock);
> 	netdev_lock(psd->main_netdev);
> 	...
> }
>
> Suppose a driver calls psp_dev_unregister() under netdev_lock() while the
> work is blocked in netdev_lock(). Then disable_delayed_work_sync() never
> returns. Taking psd->lock after that would also invert the psd->lock ->
> instance lock order.
>
> The psp_dev_unregister() kernel-doc has no Context: line covering this.
> The @tx_key_add / @tx_key_del kernel-doc in include/net/psp/types.h still
> only says "Can be left NULL if device does not store Tx keys". It doesn't
> say that psp_dev_create() now rejects tx_key_add on devices that aren't
> ops locked, and it doesn't mention the BQL requirements.
>
> mlx5 calls psp_dev_unregister() from mlx5e_nic_cleanup() without holding
> netdev_lock, so it isn't affected today.
>
> [ ... ]
>
>> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
>> index d497f41c483e1..48bffe088c79b 100644
>> --- a/net/psp/psp_sock.c
>> +++ b/net/psp/psp_sock.c
>
> [ ... ]
>
>> @@ -287,10 +293,6 @@ psp_sock_tx_rekey(struct sock *sk, struct psp_dev *psd, struct psp_assoc *pas,
>>  	struct psp_assoc *new;
>>  	int err;
>>  
>> -	if (psp_dev_has_sadb(psd)) {
>> -		NL_SET_ERR_MSG(extack, "Tx rekey not supported on this device");
>> -		return -EOPNOTSUPP;
>> -	}
>>  	if (!pas->peer_tx) {
>>  		NL_SET_ERR_MSG(extack, "Socket PSP state is not fully established");
>>  		return -EBUSY;
>
> [Severity: Medium]
> With this check removed, can an unprivileged user use up the device's Tx
> key space by calling tx-assoc in a loop on a single socket?
>
> The PSP_CMD_TX_ASSOC op in psp-nl-gen.c sets only GENL_CMD_CAP_DO and not
> GENL_ADMIN_PERM. The caller just needs to own an established TCP socket
> routed over an SADB device. Every rekey allocates a new device key:
>
> psp_nl_tx_assoc_doit()
>   psp_sock_assoc_set_tx()
>     psp_sock_tx_rekey()
>       psp_assoc_set_tx()
>         psp_dev_tx_key_add()
>           mlx5e_psp_assoc_add()
>             mlx5_create_encryption_key()
>
> The superseded key is released only after all of these steps:
>
> - an RCU grace period
> - the free work runs
> - psp_deferred_del_queue() moves it onto psd->tx_del.next
> - a BQL grace period completes on every Tx queue
>
> psp_deferred_del_work() checks for the end of that grace period only once
> every PSP_TX_GRACE_POLL_MS (1000 ms).
>
> Nothing limits the rekey rate or the length of tx_del.next and
> tx_del.active:
>
> psp_deferred_del_queue() {
> 	...
> 	list_move_tail(&pas->assocs_list, &psd->tx_del.next);
> 	schedule_delayed_work(&psd->tx_del.work, 0);
> }
>
> If a Tx queue stops completing its snapshotted bytes (for example, the
> peer sends link-level pause frames), the grace period never ends. In that
> case tx_del.next keeps growing and new device keys keep being allocated.
>
> Every PSP socket on the device shares these keys, and they aren't charged
> to the caller's memcg (only the psp_assoc allocation uses
> GFP_KERNEL_ACCOUNT). Once they run out, tx_key_add() fails for every
> other user of the device.
>
> The commit message says:
>
>   Adding a time based rate limit per socket for tx-assoc to prevent
>   wasting SADB or deferred deletion queue space is possible for future
>   work. For now, users should avoid presenting PSP devices to users they
>   don't trust to abuse this.
>
> Before this patch, a socket on an SADB device could hold only one Tx key.
> There also doesn't seem to be an in-kernel way to limit tx-assoc to
> trusted users. The rest of the series only adds selftests, so no later
> patch adds a bound.
>
> Could this patch cap the number of pending deferred deletions per device
> or per socket, or rate limit Tx rekey, instead of leaving that for later?
>

The cover letter and commit message discuss this. I think adding a rate
limit is a reasonable bit of hardening that can be done. My preference
is to introduce that in a follow up series, as I think this series is
already pretty dense in terms of complexity, and deployment-wise the
admin user can hide PSP devices with namespaces, or block all usage of
tx/rx assoc netlink calls by just disabling PSP on the device.

> [ ... ]