Re: [PATCH net 2/5] rxrpc: Fix use of wrong skb when comparing queued RESP challenge serial

[Jakub Kicinski <kuba@kernel.org>]

Hi David!

The review under --- is AI generated. It looks legit to me.

The Google reviewer also points out preexiting problems that may be worth looking at:
https://sashiko.dev/#/patchset/20260319150150.4189381-1-dhowells%40redhat.com

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
rxrpc: Fix use of wrong skb when comparing queued RESP challenge serial

In rxrpc_post_response(), the code should be comparing the challenge
serial number from the cached response before deciding to switch to a
newer response, but looks at the newer packet private data instead,
rendering the comparison always false.

Fix this by switching to look at the older packet.

> diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c
> index 98ad9b51ca2c..2c27f47951f9 100644
> --- a/net/rxrpc/conn_event.c
> +++ b/net/rxrpc/conn_event.c
> @@ -557,7 +557,7 @@ void rxrpc_post_response(struct rxrpc_connection *conn, struct sk_buff *skb)
>  	spin_lock_irq(&local->lock);
>  	old = conn->tx_response;
>  	if (old) {
> -		struct rxrpc_skb_priv *osp = rxrpc_skb(skb);
> +		struct rxrpc_skb_priv *osp = rxrpc_skb(old);
>
>  		/* Always go with the response to the most recent challenge. */
>  		if (after(sp->resp.challenge_serial, osp->resp.challenge_serial))
>  			conn->tx_response = old;
                        ^^^^
Does this have the logic inverted? The comment says "Always go with
the response to the most recent challenge" and the condition checks
if the new response has a more recent serial number:

    if (after(sp->resp.challenge_serial, osp->resp.challenge_serial))

When this is TRUE (new serial is after old serial), the code keeps
the OLD response by setting conn->tx_response = old. Shouldn't it
keep the NEW response instead? Something like:

    if (after(sp->resp.challenge_serial, osp->resp.challenge_serial))
        conn->tx_response = skb;

This would cause the connection to respond to outdated challenges
rather than the most recent one.

>  		else
>  			old = skb;

When a new response is rejected here (because the old response has a
more recent challenge serial), what happens to the rejected packet?
The function assigns it to the local variable 'old' but never frees
it.

Looking at the callers:
- rxkad_respond_to_challenge() in rxkad.c sets response = NULL after
  calling this function
- rxgk_construct_response() in rxgk.c does the same

This suggests ownership transfers to rxrpc_post_response(). Should
the rejected packet be freed with rxrpc_free_skb() before returning?

>  	} else {
>  		conn->tx_response = skb;
>  	}
>  	spin_unlock_irq(&local->lock);
>  	rxrpc_poke_conn(conn, rxrpc_conn_get_poke_response);
>  }
-- 
pw-bot: cr