From: Alok Tiwari <alok.a.tiwari@oracle.com>

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.

Fixes: 5800b1cf3fd8 ("rxrpc: Allow CHALLENGEs to the passed to the app for a RESPONSE")
Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: netdev@vger.kernel.org
cc: stable@kernel.org
---
 net/rxrpc/conn_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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))

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

Jakub Kicinski <kuba@kernel.org> wrote:

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

I saw that there was one on patchwork, but I couldn't access it.

Thanks,
David

On Sat, 21 Mar 2026 08:15:19 +0000 David Howells wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > The review under --- is AI generated. It looks legit to me.  
> 
> I saw that there was one on patchwork, but I couldn't access it.

There's a 24h delay before results are public otherwise noobs spam
the list with 100 versions until the AI is happy.

Btw, do you know how to credit the AI site with reporting something?  For
instance for syzbot gives me a Reported-by: line that carries something that
can be used as an email address.  There were other things in the AI report,
some of which I added patches for.

David

On Mon, 23 Mar 2026 21:51:08 +0000 David Howells wrote:
> Btw, do you know how to credit the AI site with reporting something?  For
> instance for syzbot gives me a Reported-by: line that carries something that
> can be used as an email address.  There were other things in the AI report,
> some of which I added patches for.

I was also wondering about that. syzbot reports are for stuff that's
already committed. Most of the CI and AI reports are pre-commit.
IDK if we have a way to attribute issues caught before commits go in.
I guess AI reviews are an integral part of the process now. We don't
have a tag for when reviewers/maintainers catch bugs in patches either.