Re: [PATCH net-next v3 5/5] selftests: drv-net: psp: add rekey tests

[Daniel Zahka]

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] In tools/testing/selftests/drivers/net/psp.py, the second half of
>   rekey_both_sides() (the "# now, tx then rx" block, lines 734-736) does…
>
> --
>
> selftests: drv-net: psp: add rekey tests
>
> This commit adds psp.py selftests for rekeying PSP connections. They cover
> basic Rx and Tx rekeys, plus cases where rx-assoc and tx-assoc should fail
> because of a psp version mismatch or the psp state. It also adds rx-assoc,
> tx-assoc and key-rotate RPCs to psp_responder.
>
>> diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
>> index 71eeade62ad24..004faf4bb57ae 100755
>> --- a/tools/testing/selftests/drivers/net/psp.py
>> +++ b/tools/testing/selftests/drivers/net/psp.py
>
> [ ... ]
>
>> @@ -489,6 +558,186 @@ def _data_basic_send(cfg, version, ipver):
>
> [ ... ]
>
>> +@ksft_variants(_get_psp_ver_variants())
>> +def rekey_both_sides(cfg, version):
>> +    """Test rekeying both directions"""
>> +    _init_psp_dev(cfg)
>> +
>> +    s = _establish_psp_conn(cfg, version)
>> +    try:
>> +        data_len = _psp_txrx(cfg, s, 10)
>> +
>> +        cfg.pspnl.key_rotate({"id": cfg.psp_dev_id})
>> +        _remote_key_rotate(cfg)
>> +
>> +        # first, rx then tx
>> +        data_len = _rekey_rx(cfg, s, version, data_len)
>> +        data_len = _rekey_tx(cfg, s, data_len)
>> +
>> +        cfg.pspnl.key_rotate({"id": cfg.psp_dev_id})
>> +        _remote_key_rotate(cfg)
>> +
>> +        # now, tx then rx
>> +        data_len = _rekey_tx(cfg, s, data_len)
>> +        _rekey_rx(cfg, s, version, data_len)
>
> [Severity: Low]
> Does the "tx then rx" half of rekey_both_sides() check that either of its
> Tx rekeys actually took effect?
>
> After an Rx rekey, the old SPI stays acceptable:
>
> net/psp/psp_sock.c:psp_sock_rx_rekey() {
>     ...
> 	pas->prev_spi = prev->rx.spi;
> 	pas->prev_generation = prev->generation;
>
> 	memcpy(&pas->tx, &prev->tx, sizeof(pas->tx));
>     ...
> }
>
> psp_pse_matches_pas() accepts packets that match it:
>
> include/net/psp/functions.h:psp_pse_matches_pas() {
>     ...
> 		(pas->prev_spi && pas->prev_spi == pse->spi &&
> 		 pas->prev_generation == pse->generation));
> }
>
> The old SPI only becomes invalid when the device key is rotated:
>
> net/psp/psp_sock.c:psp_assocs_key_rotated() {
>     ...
> 	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;
> 	}
>     ...
> }
>
> In the first half, the key_rotate calls that come next cause a stale Tx
> SPI to be dropped. The second half ends right after _rekey_rx(), and
> neither side rotates again.
>
> Say the local tx_assoc in _rekey_tx() were silently ignored. The local
> side would keep sending with the old SPI, and the responder would still
> accept it through prev_spi. The same applies if the responder ignored
> the tx assoc sent by _remote_tx_assoc() in _rekey_rx(): its echo would
> still be accepted locally.
>
> This half is also the only place where an Rx rekey follows a Tx rekey on
> the same socket, which is the memcpy of prev->tx shown above. If that
> path lost the Tx key that was just installed, would this test notice?
>
> The v3 changelog says the basic tests now "rotate again after it to
> verify the new key is in use". Could this test end the same way, with a
> final cfg.pspnl.key_rotate() and _remote_key_rotate() and then another
> _psp_txrx()?
>
>> +    finally:
>> +        _close_psp_conn(cfg, s)
>
> [ ... ]

Like you said, the other tests were changed in v3 to pick up this
coverage. This test was more about crossing device key rotations. I
think we can let the dedicated rx and tx rekey tests cover this.