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] Data race on the new field psp_assoc::prev_generation.
--
psp: support rx rekey operation
This commit lets a socket that already has PSP state install a new rx
key. The new psp_assoc copies the existing tx state and keeps the
previous rx spi and generation as a fallback. Packets matching either
the current or the previous rx spi/generation are accepted.
> diff --git a/include/net/psp/functions.h b/include/net/psp/functions.h
> index b23c30898389c..fb80e15e43681 100644
> --- a/include/net/psp/functions.h
> +++ b/include/net/psp/functions.h
> @@ -77,10 +77,12 @@ psp_is_allowed_nondata(struct sk_buff *skb, struct
> psp_assoc *pas)
> static inline bool
> psp_pse_matches_pas(struct psp_skb_ext *pse, struct psp_assoc *pas)
> {
> - return pse && pas->rx.spi == pse->spi &&
> - pas->generation == pse->generation &&
> - pas->version == pse->version &&
> - pas->dev_id == pse->dev_id;
> + return pse && pas->version == pse->version &&
> + pas->dev_id == pse->dev_id &&
> + ((pas->rx.spi == pse->spi &&
> + pas->generation == pse->generation) ||
> + (pas->prev_spi && pas->prev_spi == pse->spi &&
> + pas->prev_generation == pse->generation));
> }
[ ... ]
> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index a6b1c42dd626f..b5887171c84e7 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c
[ ... ]
> @@ -304,6 +338,10 @@ void psp_assocs_key_rotated(struct psp_dev *psd)
> pas->generation |= ~PSP_GEN_VALID_MASK;
> psd->stats.stales++;
> }
> +
> + list_for_each_entry(pas, &psd->active_assocs, assocs_list)
> + pas->prev_generation |= ~PSP_GEN_VALID_MASK;
> +
[Severity: Low]
Is this new write to prev_generation racing with the reader in
psp_pse_matches_pas()?
psp_assocs_key_rotated() runs with only psd->lock held, which is taken
in the key-rotate pre_doit before psp_nl_key_rotate_doit() calls it.
It does a plain read-modify-write on associations that are already
published through sk->psp_assoc.
The new prev_generation comparison in psp_pse_matches_pas() runs on the
receive path without psd->lock:
tcp_v4_do_rcv() / tcp_v6_do_rcv()
psp_sk_rx_policy_check()
__psp_sk_rx_policy_check()
psp_pse_matches_pas()
psp_twsk_rx_policy_check() also reads it with only RCU held.
Neither side uses READ_ONCE()/WRITE_ONCE() or data_race(). KCSAN would
probably report this whenever a device key rotation overlaps PSP rx on
a socket that has been rekeyed. Since this is a u8 there is no tearing,
so the worst effect is that a reader briefly still accepts packets on
the previous spi during a rotation.
pas->generation already has the same unannotated pattern, but this
patch adds a second field and a new write loop over active, published
associations. Should these new accesses be annotated with
WRITE_ONCE()/READ_ONCE() or data_race()?
> list_splice_init(&psd->prev_assocs, &psd->stale_assocs);
> list_splice_init(&psd->active_assocs, &psd->prev_assocs);
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-psp-v2-0-5596ab50f677%40gmail.com
