Re: [PATCH net-next v3 1/5] psp: support rx rekey operation

["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 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.
>
>> 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));
>>  }
>
> [ ... ]
>
>> 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
>
> [ ... ]
>
>> @@ -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);

This was a preexisting issue. I didn't want to move the exsiting
read/write pair to READ/WRITE ONCE as part of this patch. I prefer to
change all of them in a follow up. Also, as the bot notes, the field is
a u8, so tearing isn't possible.