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] In this commit, psp_sock_tx_rekey() (net/psp/psp_sock.c) refuses
a Tx rekey with -EOPNOTSUPP on every SADB device, i.e. any device…
- [Low] The new 'Rekeying' section in Documentation/networking/psp.rst
says a connection in 'PSP Full' state can be rekeyed in place with rx-…
- [Low] The new 'Rekeying' section in Documentation/networking/psp.rst
says the SPI in use before an Rx rekey "remains acceptable on the…
--
psp: support tx rekey operation
This patch adds a Tx rekey path to psp_sock_assoc_set_tx(). The new
psp_sock_tx_rekey() allocates a new psp_assoc that copies the Rx state of
the current assoc and installs the new Tx key. It then swaps the new assoc
into sk->psp_assoc with RCU. The patch also adds a Rekeying section to
Documentation/networking/psp.rst.
> diff --git a/Documentation/networking/psp.rst
> b/Documentation/networking/psp.rst
> index 0f9b6b73f2447..5c4e4215d906f 100644
> --- a/Documentation/networking/psp.rst
> +++ b/Documentation/networking/psp.rst
> @@ -140,14 +140,36 @@ The PSP assoc state of a socket is not reset when the
> connection is
> torn down. ``connect()`` on a socket that has PSP assoc state will
> return ``-EINVAL``.
>
> +Rekeying
> +--------
> +
> +A connection which has completed the exchange described above and is
> +in the "PSP Full" state can be rekeyed in place without being torn
> +down. The Tx and Rx directions are rekeyed separately using the same
> +netlink calls as in connection setup.
> +
> +``rx-assoc`` allocates a new Rx key and SPI, which should be passed
> +to the peer exactly as during the initial exchange. The SPI which was
> +in use before the rekey remains acceptable on the socket until the
> +next Rx rekey, so packets the peer sent before it learned the new SPI
> +are still received.
[Severity: Low]
Is "until the next Rx rekey" accurate? A device key rotation seems to end
acceptance of the previous SPI as well.
After an Rx rekey, psp_assoc_create() puts the new assoc on
psd->active_assocs, with prev_generation copied from the old assoc. On the
next rotation, psp_assocs_key_rotated() runs:
list_for_each_entry(pas, &psd->active_assocs, assocs_list)
pas->prev_generation |= ~PSP_GEN_VALID_MASK;
psp_pse_matches_pas() only accepts the previous SPI when the generation
also matches:
(pas->prev_spi && pas->prev_spi == pse->spi &&
pas->prev_generation == pse->generation)
So after the next device key rotation, packets that use the previous SPI
would be dropped, even if there has been no further Rx rekey.
Should the doc say that the old SPI also stops being accepted at the next
device key rotation?
> +
> +``tx-assoc`` installs a new Tx key, which takes effect immediately.
> +The peer will accept data encrypted from the old and new SPI.
> +
> +A rekey is rejected with ``-EINVAL`` if it would change the PSP device
> +or the PSP version of the association. It is rejected with ``-EBUSY``
> +if the socket has not completed its initial exchange in both
> +directions.
[Severity: Low]
Does this list cover every -EINVAL case for an Rx rekey?
psp_sock_rx_rekey() also rejects a new SPI whose key phase bit matches the
current one:
if (!((pas->rx.spi ^ prev->rx.spi) & cpu_to_be32(PSP_SPI_KEY_PHASE))) {
NL_SET_ERR_MSG(extack, "New and prev SPI have same phase bit");
return -EINVAL;
}
Drivers set the phase bit from the current device key. In netdevsim, for
example, nsim_rx_spi_alloc() stays inside the current phase, and only
nsim_key_rotate() flips PSP_SPI_KEY_PHASE.
If the device key has not rotated since the current SPI was allocated, an
rx-assoc rekey will always fail with -EINVAL. An application that rekeys on
its own schedule, instead of after a rotation notification, would hit this.
Could this requirement be documented here? The text is still the same at
the end of the series.
[ ... ]
> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index b5887171c84e7..060149f3d72a7 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c
> @@ -283,6 +283,52 @@ psp_sock_set_tx_key(struct sock *sk, struct psp_dev
> *psd, struct psp_assoc *pas,
> return err;
> }
>
> +static int
> +psp_sock_tx_rekey(struct sock *sk, struct psp_dev *psd, struct psp_assoc
> *pas,
> + struct psp_key_parsed *key, struct netlink_ext_ack *extack)
> +{
> + 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;
> + }
[Severity: Low]
At this commit, should this limitation be mentioned in the commit message
or in the new Rekeying section?
psp_dev_has_sadb() returns true whenever psd->ops->tx_key_del is set. That
includes mlx5, the only in-tree hardware driver: mlx5_psp_ops sets
.tx_key_del = mlx5e_psp_assoc_del.
So on mlx5 this call chain returns -EOPNOTSUPP:
psp_nl_tx_assoc_doit()->psp_sock_assoc_set_tx()->psp_sock_tx_rekey()
However, psp.rst lists only -EINVAL and -EBUSY as reasons a rekey can be
rejected.
The next patch in the series, "psp: defer tx key deletions for SADB
drivers", removes this check and sets PSP_ASSOC_DEFER_TX_KEY_DEL on the
old assoc instead. The mismatch therefore only exists partway through the
series.
> + if (!pas->peer_tx) {
> + NL_SET_ERR_MSG(extack, "Socket PSP state is not fully
> established");
> + return -EBUSY;
> + }
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-psp-v2-0-5596ab50f677%40gmail.com
