Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] psp_dev_tx_key_del() (net/psp/psp_sock.c) calls
psd->ops->tx_key_del(psd, pas) and then always decrements…
--
psp: add core tracked stat for outstanding tx keys
This commit adds a core-maintained tx_key_cnt to psd->stats. The counter
goes up when tx_key_add() succeeds and down in psp_dev_tx_key_del(). It is
reported to userspace as tx-key-count, but only for drivers that use an
SADB, and psp_dev_unregister() warns if any keys are still outstanding.
> diff --git a/Documentation/netlink/specs/psp.yaml
> b/Documentation/netlink/specs/psp.yaml
> index f3266763c3253..d14c11d098914 100644
> --- a/Documentation/netlink/specs/psp.yaml
> +++ b/Documentation/netlink/specs/psp.yaml
> @@ -190,6 +190,13 @@ attribute-sets:
> doc: |
> Number of PSP packets for transmission with errors.
> Device statistic (from the PSP spec).
> + -
> + name: tx-key-count
> + type: uint
> + doc: |
> + Current number of Tx keys installed on the device.
> + Only visible if the driver stores keys on device.
> + Kernel statistic.
[ ... ]
> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index d21f7afd830a3..c458bc551b4b9 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c
[ ... ]
> @@ -105,6 +107,8 @@ static int psp_dev_tx_key_add(struct psp_dev *psd, struct
> psp_assoc *pas,
> void psp_dev_tx_key_del(struct psp_dev *psd, struct psp_assoc *pas)
> {
> psd->ops->tx_key_del(psd, pas);
> + if (!WARN_ON_ONCE(!psd->stats.tx_key_cnt))
> + psd->stats.tx_key_cnt--;
> }
[Severity: Low]
Is tx-key-count really the number of keys installed on the device? The
YAML doc says "Current number of Tx keys installed on the device". The
commit message says it counts keys "from the hw's perspective" and can show
whether "a device is approaching its SADB capacity".
struct psp_dev_ops::tx_key_del returns void, so psp_dev_tx_key_del()
decrements the counter whether or not the device actually freed the entry.
In mlx5, which is the only SADB driver, the delete path drops any firmware
error:
drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp.c:mlx5e_psp_assoc_del() {
...
nkey = (struct psp_key *)pas->drv_data;
mlx5_destroy_encryption_key(priv->mdev, nkey->id);
atomic_dec(&psp->tx_key_cnt);
}
drivers/net/ethernet/mellanox/mlx5/core/lib/crypto.c:mlx5_crypto_destroy_dek_key()
{
...
mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));
}
The call path is psp_assoc_free(), psp_deferred_del_work() or
psp_deferred_del_uninit(), then psp_dev_tx_key_del(), then
mlx5e_psp_assoc_del(), then mlx5_destroy_encryption_key().
If MLX5_CMD_OP_DESTROY_GENERAL_OBJECT fails (bad status or timeout), the
DEK can stay allocated on the device while tx_key_cnt still goes down. In
that case the stat would read lower than real SADB occupancy.
The counter still catches missing tx_key_del calls in the core. It does
not catch failed deletes on the device side.
Should the doc and commit message describe this as a count of keys the
stack believes are installed? The alternative would be for tx_key_del to
return an error, so the core can skip the decrement when the device-side
delete fails.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-psp-v2-0-5596ab50f677%40gmail.com
