[PATCH net 0/5] rxrpc: Miscellaneous fixes

[PATCH net 0/5] rxrpc: Miscellaneous fixes

[David Howells]

Here are some fixes for AF_RXRPC:

 (1) Fix listen() allowing preallocation to overrun the prealloc buffer.

 (2) Prevent resending the request if we've seen the reply starting to
     arrive.

 (3) Fix accidental sharing of ACK state between transmission and
     reception.

 (4) Ignore ACKs in which ack.previousPacket regresses.  This indicates the
     highest DATA number so far seen, so should not be seen to go
     backwards.

 (5) Fix the determination of when to generate an IDLE-type ACK,
     simplifying it so that we generate one if we have more than two DATA
     packets that aren't hard-acked (consumed) or soft-acked (in the rx
     buffer, but could be discarded and re-requested).

The patches are tagged here:

	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
	rxrpc-fixes-20220521

and can also be found on the following branch:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

Tested-by: kafs-testing+fedora34_64checkkafs-build-495@auristor.com

Changes
=======
ver #2)
 - Rebased onto net/master
 - Dropped the IPv6 checksum patch as it's already upstream.

David

Link: https://lore.kernel.org/r/165306442115.34086.1818959430525328753.stgit@warthog.procyon.org.uk/ # v1
---
David Howells (5):
      rxrpc: Fix listen() setting the bar too high for the prealloc rings
      rxrpc: Don't try to resend the request if we're receiving the reply
      rxrpc: Fix overlapping ACK accounting
      rxrpc: Don't let ack.previousPacket regress
      rxrpc: Fix decision on when to generate an IDLE ACK


 include/trace/events/rxrpc.h |  2 +-
 net/rxrpc/ar-internal.h      | 13 +++++++------
 net/rxrpc/call_event.c       |  3 ++-
 net/rxrpc/input.c            | 31 ++++++++++++++++++++-----------
 net/rxrpc/output.c           | 20 ++++++++++++--------
 net/rxrpc/recvmsg.c          |  8 +++-----
 net/rxrpc/sysctl.c           |  4 ++--
 7 files changed, 47 insertions(+), 34 deletions(-)

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[David Miller]

From: David Howells <dhowells@redhat.com>
Date: Sat, 21 May 2022 09:02:58 +0100

> 
> Here are some fixes for AF_RXRPC:
> 
>  (1) Fix listen() allowing preallocation to overrun the prealloc buffer.
> 
>  (2) Prevent resending the request if we've seen the reply starting to
>      arrive.
> 
>  (3) Fix accidental sharing of ACK state between transmission and
>      reception.
> 
>  (4) Ignore ACKs in which ack.previousPacket regresses.  This indicates the
>      highest DATA number so far seen, so should not be seen to go
>      backwards.
> 
>  (5) Fix the determination of when to generate an IDLE-type ACK,
>      simplifying it so that we generate one if we have more than two DATA
>      packets that aren't hard-acked (consumed) or soft-acked (in the rx
>      buffer, but could be discarded and re-requested).
> 
> The patches are tagged here:
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git
> 	rxrpc-fixes-20220521
> 
> and can also be found on the following branch:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

I tried to pull from this url and it does not work, just fyi...

So I applied the series instead.

> Tested-by: kafs-testing+fedora34_64checkkafs-build-495@auristor.com

Thank you.

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Sat, 21 May 2022 09:02:58 +0100 you wrote:
> Here are some fixes for AF_RXRPC:
> 
>  (1) Fix listen() allowing preallocation to overrun the prealloc buffer.
> 
>  (2) Prevent resending the request if we've seen the reply starting to
>      arrive.
> 
> [...]

Here is the summary with links:
  - [net,1/5] rxrpc: Fix listen() setting the bar too high for the prealloc rings
    https://git.kernel.org/netdev/net/c/88e22159750b
  - [net,2/5] rxrpc: Don't try to resend the request if we're receiving the reply
    https://git.kernel.org/netdev/net/c/114af61f88fb
  - [net,3/5] rxrpc: Fix overlapping ACK accounting
    https://git.kernel.org/netdev/net/c/8940ba3cfe48
  - [net,4/5] rxrpc: Don't let ack.previousPacket regress
    https://git.kernel.org/netdev/net/c/81524b631253
  - [net,5/5] rxrpc: Fix decision on when to generate an IDLE ACK
    https://git.kernel.org/netdev/net/c/9a3dedcf1809

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net 0/5] rxrpc: Miscellaneous fixes

[David Howells]

Here some miscellaneous fixes for AF_RXRPC:

 (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut
     ssthresh when the peer cuts its rwind size.

 (2) Only transmit a single ACK for all the DATA packets glued together
     into a jumbo packet to reduce the number of ACKs being generated.

 (3) Clean up the generation of flags in the protocol header when creating
     a packet for transmission.  This means we don't carry the old
     REQUEST-ACK bit around from previous transmissions, will make it
     easier to fix the MORE-PACKETS flag and make it easier to do jumbo
     packet assembly in future.

 (4) Fix how the MORE-PACKETS flag is driven.  We shouldn't be setting it
     in sendmsg() as the packet is then queued and the bit is left in that
     state, no matter how long it takes us to transmit the packet - and
     will still be in that state if the packet is retransmitted.

 (5) Request an ACK on an impending transmission stall due to the app layer
     not feeding us new data fast enough.  If we don't request an ACK, we
     may have to hold on to the packet buffers for a significant amount of
     time until the receiver gets bored and sends us an ACK anyway.

David

---
The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

David Howells (5):
  rxrpc: Fix congestion control algorithm
  rxrpc: Only transmit one ACK per jumbo packet received
  rxrpc: Clean up Tx header flags generation handling
  rxrpc: Change how the MORE-PACKETS rxrpc wire header flag is driven
  rxrpc: Request an ACK on impending Tx stall

 include/trace/events/rxrpc.h |  2 +-
 net/rxrpc/ar-internal.h      |  2 +-
 net/rxrpc/call_object.c      |  7 +-----
 net/rxrpc/input.c            | 49 +++++++++++++++++++++++++-----------
 net/rxrpc/output.c           | 26 ++++++++++++++-----
 net/rxrpc/proc.c             |  6 ++---
 net/rxrpc/sendmsg.c          |  3 ---
 7 files changed, 61 insertions(+), 34 deletions(-)

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[Jakub Kicinski]

On Fri,  3 May 2024 16:07:38 +0100 David Howells wrote:
> Here some miscellaneous fixes for AF_RXRPC:
> 
>  (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut
>      ssthresh when the peer cuts its rwind size.
> 
>  (2) Only transmit a single ACK for all the DATA packets glued together
>      into a jumbo packet to reduce the number of ACKs being generated.
> 
>  (3) Clean up the generation of flags in the protocol header when creating
>      a packet for transmission.  This means we don't carry the old
>      REQUEST-ACK bit around from previous transmissions, will make it
>      easier to fix the MORE-PACKETS flag and make it easier to do jumbo
>      packet assembly in future.
> 
>  (4) Fix how the MORE-PACKETS flag is driven.  We shouldn't be setting it
>      in sendmsg() as the packet is then queued and the bit is left in that
>      state, no matter how long it takes us to transmit the packet - and
>      will still be in that state if the packet is retransmitted.
> 
>  (5) Request an ACK on an impending transmission stall due to the app layer
>      not feeding us new data fast enough.  If we don't request an ACK, we
>      may have to hold on to the packet buffers for a significant amount of
>      time until the receiver gets bored and sends us an ACK anyway.

Looks like these got marked as Rejected in patchwork.
I think either because lore is confused and attaches an exchange with
DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm
not sure these are fixes. So let me ask - on a scale of 1 to 10, how
convinced are you that these should go to Linus this week rather than
being categorized as general improvements and go during the merge
window (without the Fixes tags)?

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[Jeffrey Altman]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]


> On May 7, 2024, at 8:44 PM, Jakub Kicinski <kuba@kernel.org> wrote:
> 
> On Fri,  3 May 2024 16:07:38 +0100 David Howells wrote:
>> Here some miscellaneous fixes for AF_RXRPC:
>> 
>> (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut
>>  ssthresh when the peer cuts its rwind size.
>> 
>> (2) Only transmit a single ACK for all the DATA packets glued together
>>  into a jumbo packet to reduce the number of ACKs being generated.
>> 
>> (3) Clean up the generation of flags in the protocol header when creating
>>  a packet for transmission.  This means we don't carry the old
>>  REQUEST-ACK bit around from previous transmissions, will make it
>>  easier to fix the MORE-PACKETS flag and make it easier to do jumbo
>>  packet assembly in future.
>> 
>> (4) Fix how the MORE-PACKETS flag is driven.  We shouldn't be setting it
>>  in sendmsg() as the packet is then queued and the bit is left in that
>>  state, no matter how long it takes us to transmit the packet - and
>>  will still be in that state if the packet is retransmitted.
>> 
>> (5) Request an ACK on an impending transmission stall due to the app layer
>>  not feeding us new data fast enough.  If we don't request an ACK, we
>>  may have to hold on to the packet buffers for a significant amount of
>>  time until the receiver gets bored and sends us an ACK anyway.
> 
> Looks like these got marked as Rejected in patchwork.
> I think either because lore is confused and attaches an exchange with
> DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm
> not sure these are fixes. So let me ask - on a scale of 1 to 10, how
> convinced are you that these should go to Linus this week rather than
> being categorized as general improvements and go during the merge
> window (without the Fixes tags)?

Jakub,

In my opinion, the first two patches in the series I believe are important to back port to the stable branches.

Reviewed-by: Jeffrey Altman <jaltman@auristor.com <mailto:jaltman@auristor.com>>

Jeffrey




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3929 bytes --]

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[Jakub Kicinski]

On Wed, 8 May 2024 01:57:43 -0600 Jeffrey Altman wrote:
> > Looks like these got marked as Rejected in patchwork.
> > I think either because lore is confused and attaches an exchange with
> > DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm
> > not sure these are fixes. So let me ask - on a scale of 1 to 10, how
> > convinced are you that these should go to Linus this week rather than
> > being categorized as general improvements and go during the merge
> > window (without the Fixes tags)?  
> 
> Jakub,
> 
> In my opinion, the first two patches in the series I believe are important to back port to the stable branches.
> 
> Reviewed-by: Jeffrey Altman <jaltman@auristor.com <mailto:jaltman@auristor.com>>

Are they regressions? Seems possible from the Fixes tag but unclear
from the text of the commit messages.

In any case, taking the first two may be a reasonable compromise.
Does it sounds good to you, David?

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[David Howells]

Jakub Kicinski <kuba@kernel.org> wrote:

> Looks like these got marked as Rejected in patchwork.
> I think either because lore is confused and attaches an exchange with
> DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm
> not sure these are fixes. So let me ask - on a scale of 1 to 10, how
> convinced are you that these should go to Linus this week rather than
> being categorized as general improvements and go during the merge
> window (without the Fixes tags)?

Ah, sorry.  I marked them rejected as I put myself as cc: not S-o-b on one of
them, but then got distracted and didn't get around to reposting them.  And
Jeff mentioned that the use of the MORE-PACKETS flag is not exactly
consistent between various implementations.

So if you could take just the first two for the moment?

Thanks,
David

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[Jakub Kicinski]

On Wed, 08 May 2024 15:00:28 +0100 David Howells wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > Looks like these got marked as Rejected in patchwork.
> > I think either because lore is confused and attaches an exchange with
> > DaveM from 2022 to them (?) or because I mentioned to DaveM that I'm
> > not sure these are fixes. So let me ask - on a scale of 1 to 10, how
> > convinced are you that these should go to Linus this week rather than
> > being categorized as general improvements and go during the merge
> > window (without the Fixes tags)?  
> 
> Ah, sorry.  I marked them rejected as I put myself as cc: not S-o-b on one of
> them, but then got distracted and didn't get around to reposting them.  And
> Jeff mentioned that the use of the MORE-PACKETS flag is not exactly
> consistent between various implementations.

Ah, mystery solved :)

> So if you could take just the first two for the moment?

Done!

Re: [PATCH net 0/5] rxrpc: Miscellaneous fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri,  3 May 2024 16:07:38 +0100 you wrote:
> Here some miscellaneous fixes for AF_RXRPC:
> 
>  (1) Fix the congestion control algorithm to start cwnd at 4 and to not cut
>      ssthresh when the peer cuts its rwind size.
> 
>  (2) Only transmit a single ACK for all the DATA packets glued together
>      into a jumbo packet to reduce the number of ACKs being generated.
> 
> [...]

Here is the summary with links:
  - [net,1/5] rxrpc: Fix congestion control algorithm
    https://git.kernel.org/netdev/net/c/ba4e103848d3
  - [net,2/5] rxrpc: Only transmit one ACK per jumbo packet received
    https://git.kernel.org/netdev/net/c/012b7206918d
  - [net,3/5] rxrpc: Clean up Tx header flags generation handling
    (no matching commit)
  - [net,4/5] rxrpc: Change how the MORE-PACKETS rxrpc wire header flag is driven
    (no matching commit)
  - [net,5/5] rxrpc: Request an ACK on impending Tx stall
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net 0/5] rxrpc: Miscellaneous fixes

[David Howells]

Here are some fixes for rxrpc:

 (1) Fix the calling of IP routing code with IRQs disabled.

 (2) Fix a recvmsg/recvmsg race when the first completes a call.

 (3) Fix a race between notification, recvmsg and sendmsg releasing a call.

 (4) Fix abort of abort.

 (5) Fix call-level aborts that should be connection-level aborts.

David

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

David Howells (5):
  rxrpc: Fix irq-disabled in local_bh_enable()
  rxrpc: Fix recv-recv race of completed call
  rxrpc: Fix notification vs call-release vs recvmsg
  rxrpc: Fix transmission of an abort in response to an abort
  rxrpc: Fix to use conn aborts for conn-wide failures

 include/trace/events/rxrpc.h |  6 +++++-
 net/rxrpc/ar-internal.h      |  4 ++++
 net/rxrpc/call_accept.c      | 14 ++++++++------
 net/rxrpc/call_object.c      | 28 ++++++++++++----------------
 net/rxrpc/io_thread.c        | 14 ++++++++++++++
 net/rxrpc/output.c           | 22 +++++++++++++---------
 net/rxrpc/peer_object.c      |  6 ++----
 net/rxrpc/recvmsg.c          | 23 +++++++++++++++++++++--
 net/rxrpc/security.c         |  8 ++++----
 9 files changed, 83 insertions(+), 42 deletions(-)

[PATCH net 0/5] rxrpc: Miscellaneous fixes

[David Howells]

Here are some fixes for rxrpc:

 (1) Fix missing bounds checks when parsing RxGK tickets.

 (2) Fix use of wrong skbuff to get challenge serial number.

 (3) Fix unexpected RACK timer warning to report old mode.

 (4) Fix server keyring refcount leak.

 (5) Fix call key refcount leak.

David

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

Alok Tiwari (2):
  rxrpc: Fix use of wrong skb when comparing queued RESP challenge
    serial
  rxrpc: Fix rack timer warning to report unexpected mode

Anderson Nascimento (2):
  rxrpc: Fix keyring reference count leak in rxrpc_setsockopt()
  rxrpc: Fix key reference count leak from call->key

Oleh Konko (1):
  rxrpc: Fix RxGK token loading to check bounds

 net/rxrpc/af_rxrpc.c    |  2 +-
 net/rxrpc/call_object.c |  1 +
 net/rxrpc/conn_event.c  |  2 +-
 net/rxrpc/input_rack.c  |  2 +-
 net/rxrpc/key.c         | 30 +++++++++++++++++-------------
 5 files changed, 21 insertions(+), 16 deletions(-)

[PATCH net 1/5] rxrpc: Fix RxGK token loading to check bounds

[David Howells]

From: Oleh Konko <security@1seal.org>

rxrpc_preparse_xdr_yfs_rxgk() reads the raw key length and ticket length
from the XDR token as u32 values and passes each through round_up(x, 4)
before using the rounded value for validation and allocation.  When the raw
length is >= 0xfffffffd, round_up() wraps to 0, so the bounds check and
kzalloc both use 0 while the subsequent memcpy still copies the original
~4 GiB value, producing a heap buffer overflow reachable from an
unprivileged add_key() call.

Fix this by:

 (1) Rejecting raw key lengths above AFSTOKEN_GK_KEY_MAX and raw ticket
     lengths above AFSTOKEN_GK_TOKEN_MAX before rounding, consistent with
     the caps that the RxKAD path already enforces via AFSTOKEN_RK_TIX_MAX.

 (2) Sizing the flexible-array allocation from the validated raw key
     length via struct_size_t() instead of the rounded value.

 (3) Caching the raw lengths so that the later field assignments and
     memcpy calls do not re-read from the token, eliminating a class of
     TOCTOU re-parse.

The control path (valid token with lengths within bounds) is unaffected.

Fixes: 0ca100ff4df6 ("rxrpc: Add YFS RxGK (GSSAPI) security class")
Signed-off-by: Oleh Konko <security@1seal.org>
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/key.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 85078114b2dd..c96e1d8f4845 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -13,6 +13,7 @@
 #include <crypto/skcipher.h>
 #include <linux/module.h>
 #include <linux/net.h>
+#include <linux/overflow.h>
 #include <linux/skbuff.h>
 #include <linux/key-type.h>
 #include <linux/ctype.h>
@@ -171,7 +172,7 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	size_t plen;
 	const __be32 *ticket, *key;
 	s64 tmp;
-	u32 tktlen, keylen;
+	size_t raw_keylen, raw_tktlen, keylen, tktlen;
 
 	_enter(",{%x,%x,%x,%x},%x",
 	       ntohl(xdr[0]), ntohl(xdr[1]), ntohl(xdr[2]), ntohl(xdr[3]),
@@ -181,18 +182,22 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 		goto reject;
 
 	key = xdr + (6 * 2 + 1);
-	keylen = ntohl(key[-1]);
-	_debug("keylen: %x", keylen);
-	keylen = round_up(keylen, 4);
+	raw_keylen = ntohl(key[-1]);
+	_debug("keylen: %zx", raw_keylen);
+	if (raw_keylen > AFSTOKEN_GK_KEY_MAX)
+		goto reject;
+	keylen = round_up(raw_keylen, 4);
 	if ((6 * 2 + 2) * 4 + keylen > toklen)
 		goto reject;
 
 	ticket = xdr + (6 * 2 + 1 + (keylen / 4) + 1);
-	tktlen = ntohl(ticket[-1]);
-	_debug("tktlen: %x", tktlen);
-	tktlen = round_up(tktlen, 4);
+	raw_tktlen = ntohl(ticket[-1]);
+	_debug("tktlen: %zx", raw_tktlen);
+	if (raw_tktlen > AFSTOKEN_GK_TOKEN_MAX)
+		goto reject;
+	tktlen = round_up(raw_tktlen, 4);
 	if ((6 * 2 + 2) * 4 + keylen + tktlen != toklen) {
-		kleave(" = -EKEYREJECTED [%x!=%x, %x,%x]",
+		kleave(" = -EKEYREJECTED [%zx!=%x, %zx,%zx]",
 		       (6 * 2 + 2) * 4 + keylen + tktlen, toklen,
 		       keylen, tktlen);
 		goto reject;
@@ -206,7 +211,7 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	if (!token)
 		goto nomem;
 
-	token->rxgk = kzalloc(sizeof(*token->rxgk) + keylen, GFP_KERNEL);
+	token->rxgk = kzalloc(struct_size_t(struct rxgk_key, _key, raw_keylen), GFP_KERNEL);
 	if (!token->rxgk)
 		goto nomem_token;
 
@@ -221,9 +226,9 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	token->rxgk->enctype	= tmp = xdr_dec64(xdr + 5 * 2);
 	if (tmp < 0 || tmp > UINT_MAX)
 		goto reject_token;
-	token->rxgk->key.len	= ntohl(key[-1]);
+	token->rxgk->key.len	= raw_keylen;
 	token->rxgk->key.data	= token->rxgk->_key;
-	token->rxgk->ticket.len = ntohl(ticket[-1]);
+	token->rxgk->ticket.len = raw_tktlen;
 
 	if (token->rxgk->endtime != 0) {
 		expiry = rxrpc_s64_to_time64(token->rxgk->endtime);
@@ -236,8 +241,7 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
 	memcpy(token->rxgk->key.data, key, token->rxgk->key.len);
 
 	/* Pad the ticket so that we can use it directly in XDR */
-	token->rxgk->ticket.data = kzalloc(round_up(token->rxgk->ticket.len, 4),
-					   GFP_KERNEL);
+	token->rxgk->ticket.data = kzalloc(tktlen, GFP_KERNEL);
 	if (!token->rxgk->ticket.data)
 		goto nomem_yrxgk;
 	memcpy(token->rxgk->ticket.data, ticket, token->rxgk->ticket.len);

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

[David Howells]

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

[PATCH net 3/5] rxrpc: Fix rack timer warning to report unexpected mode

[David Howells]

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

rxrpc_rack_timer_expired() clears call->rack_timer_mode to OFF before
the switch. The default case warning therefore always prints OFF and
doesn't identify the unexpected timer mode.

Log the saved mode value instead so the warning reports the actual
unexpected rack timer mode.

Fixes: 7c482665931b ("rxrpc: Implement RACK/TLP to deal with transmission stalls [RFC8985]")
Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Simon Horman <horms@kernel.org>
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: linux-afs@lists.infradead.org
cc: netdev@vger.kernel.org
cc: stable@kernel.org
---
 net/rxrpc/input_rack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/input_rack.c b/net/rxrpc/input_rack.c
index 13c371261e0a..9eb109ffba56 100644
--- a/net/rxrpc/input_rack.c
+++ b/net/rxrpc/input_rack.c
@@ -413,6 +413,6 @@ void rxrpc_rack_timer_expired(struct rxrpc_call *call, ktime_t overran_by)
 		break;
 	//case RXRPC_CALL_RACKTIMER_ZEROWIN:
 	default:
-		pr_warn("Unexpected rack timer %u", call->rack_timer_mode);
+		pr_warn("Unexpected rack timer %u", mode);
 	}
 }

[PATCH net 4/5] rxrpc: Fix keyring reference count leak in rxrpc_setsockopt()

[David Howells]

From: Anderson Nascimento <anderson@allelesecurity.com>

In rxrpc_setsockopt(), the code checks 'rx->key' when handling the
RXRPC_SECURITY_KEYRING option.  However, this appears to be a logic error.
The code should be checking 'rx->securities' to determine if a keyring has
already been defined for the socket.

Currently, if a user calls setsockopt(RXRPC_SECURITY_KEYRING) multiple
times on the same socket, the check 'if (rx->key)' fails to block
subsequent calls because 'rx->key' has not been defined by the function.
This results in a reference count leak on the keyring.

This patch changes the check to 'rx->securities' to correctly identify if
the socket security keyring has already been configured, returning -EINVAL
on subsequent attempts.

Before the patch:

It shows the keyring reference counter elevated.

$ cat /proc/keys | grep AFSkeys1
27aca8ae I--Q--- 24469721 perm 3f010000  1000  1000 keyring   AFSkeys1: empty
$

After the patch:

The keyring reference counter remains stable and subsequent calls return an
error:

$ ./poc
setsockopt: Invalid argument
$

Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Signed-off-by: Anderson Nascimento <anderson@allelesecurity.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/af_rxrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index 0f90272ac254..0b7ed99a3025 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -665,7 +665,7 @@ static int rxrpc_setsockopt(struct socket *sock, int level, int optname,
 
 		case RXRPC_SECURITY_KEYRING:
 			ret = -EINVAL;
-			if (rx->key)
+			if (rx->securities)
 				goto error;
 			ret = -EISCONN;
 			if (rx->sk.sk_state != RXRPC_UNBOUND)

[PATCH net 5/5] rxrpc: Fix key reference count leak from call->key

[David Howells]

From: Anderson Nascimento <anderson@allelesecurity.com>

When creating a client call in rxrpc_alloc_client_call(), the code obtains
a reference to the key.  This is never cleaned up and gets leaked when the
call is destroyed.

Fix this by freeing call->key in rxrpc_destroy_call().

Before the patch, it shows the key reference counter elevated:

$ cat /proc/keys | grep afs@54321
1bffe9cd I--Q--i 8053480 4169w 3b010000  1000  1000 rxrpc     afs@54321: ka
$

After the patch, the invalidated key is removed when the code exits:

$ cat /proc/keys | grep afs@54321
$

Fixes: f3441d4125fc ("rxrpc: Copy client call parameters into rxrpc_call earlier")
Signed-off-by: Anderson Nascimento <anderson@allelesecurity.com>
Co-developed-by: David Howells <dhowells@redhat.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/call_object.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/rxrpc/call_object.c b/net/rxrpc/call_object.c
index 918f41d97a2f..8d874ea428ff 100644
--- a/net/rxrpc/call_object.c
+++ b/net/rxrpc/call_object.c
@@ -694,6 +694,7 @@ static void rxrpc_destroy_call(struct work_struct *work)
 	rxrpc_put_bundle(call->bundle, rxrpc_bundle_put_call);
 	rxrpc_put_peer(call->peer, rxrpc_peer_put_call);
 	rxrpc_put_local(call->local, rxrpc_local_put_call);
+	key_put(call->key);
 	call_rcu(&call->rcu, rxrpc_rcu_free_call);
 }
 

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

[Jakub Kicinski]

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

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

[David Howells]

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

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

[Jakub Kicinski]

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.

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

[David Howells]

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

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

[Jakub Kicinski]

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.