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] There is an unannotated data race on the new
psp_assoc::prev_generation field.
--
psp: support rx rekey operation
This commit lets an established PSP socket rekey its Rx direction. It
does this by creating a new psp_assoc that copies the existing Tx state,
and it records the previous Rx SPI and generation as a fallback.
psp_pse_matches_pas() now accepts packets that match either the current
or the previous Rx SPI/generation, and psp_assocs_key_rotated()
invalidates prev_generation as needed.
quoted hunk ↗ jump to hunk
diff --git a/include/net/psp/functions.h b/include/net/psp/functions.h
index 410adb6718c16..5a7a518b9579b 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));
}
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
index f2cba3fa3a235..c1e38845f08c3 100644
--- a/net/psp/psp_sock.c
+++ b/net/psp/psp_sock.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -302,8 +333,21 @@ void psp_assocs_key_rotated(struct psp_dev *psd)
*/
list_for_each_entry_safe(pas, next, &psd->prev_assocs, assocs_list) {
pas->generation |= ~PSP_GEN_VALID_MASK;
+ pas->prev_generation |= ~PSP_GEN_VALID_MASK;
psd->stats.stales++;
}
+
+ /* Don't allow SPI from two generations ago to alias one from the
+ * current generation. Rx rekeying under the same device key generation
+ * is currently permitted, so skip if the previous SPI is from just one
+ * generation ago.
+ */
+ list_for_each_entry(pas, &psd->active_assocs, assocs_list) {
+ if ((pas->rx.spi ^ pas->prev_spi) &
+ cpu_to_be32(PSP_SPI_KEY_PHASE))
+ pas->prev_generation |= ~PSP_GEN_VALID_MASK;
+ }
[Severity: Low]
Will KCSAN report a data race on pas->prev_generation here?
psp_assocs_key_rotated() updates prev_generation with a plain
read-modify-write while holding only psd->lock. That happens both for
the prev_assocs entries and in the new active_assocs loop. Both lists
hold assocs that are still attached to sockets and can still receive
traffic.
On the Rx side, psp_pse_matches_pas() reads prev_spi and prev_generation
with plain loads and does not take 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() reaches the same check under RCU.
This means a key-rotate running while a rekeyed socket is receiving PSP
traffic gives a plain write racing with a plain read. The writer and
reader hold no lock in common.
Since the field is a u8, it can't tear. A racing packet would either
match the old prev_generation or be dropped against the invalidated one,
so there doesn't seem to be a functional problem. The existing
pas->generation update already follows the same unannotated pattern.
Would it make sense to use WRITE_ONCE() for these stores in
psp_assocs_key_rotated() and READ_ONCE() for the loads in
psp_pse_matches_pas()? The later patches in this series don't appear to
add those annotations.
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/20261106-psp-v3-0-a8dd38f5a727%40gmail.com