Netdev List
 help / color / mirror / Atom feed
From: Jeffrey E Altman <jaltman@auristor.com>
To: David Howells <dhowells@redhat.com>, netdev@vger.kernel.org
Cc: Marc Dionne <marc.dionne@auristor.com>,
	Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Paolo Abeni <pabeni@redhat.com>, Simon Horman <horms@kernel.org>,
	linux-afs@lists.infradead.org, linux-kernel@vger.kernel.org,
	Wyatt Feng <bronzed_45_vested@icloud.com>,
	stable@vger.kernel.org, Yuan Tan <yuantan098@gmail.com>,
	Yifan Wu <yifanwucs@gmail.com>,
	Juefei Pu <tomapufckgml@gmail.com>,
	Zhengchuan Liang <zcliangcn@gmail.com>, Xin Liu <bird@lzu.edu.cn>,
	Ren Wei <n05ec@lzu.edu.cn>
Subject: Re: [PATCH net v2 01/10] rxrpc: input: reject ACKALL outside transmit phase
Date: Fri, 19 Jun 2026 17:32:38 -0400	[thread overview]
Message-ID: <c0fd4fec-1576-4070-b31e-a37d5506f5ed@auristor.com> (raw)
In-Reply-To: <20260618134802.2477777-2-dhowells@redhat.com>

On 6/18/2026 9:47 AM, David Howells wrote:
> From: Wyatt Feng <bronzed_45_vested@icloud.com>
>
> rxrpc_input_ackall() accepts ACKALL packets without checking whether
> the call is in a state that can legitimately have outstanding transmit
> buffers.  A forged ACKALL can therefore reach a new service call in
> RXRPC_CALL_SERVER_RECV_REQUEST before any reply packets have been
> queued.
>
> In that state call->tx_top is zero and call->tx_queue is NULL, so
> rxrpc_rotate_tx_window() dereferences a NULL txqueue and triggers a
> null-pointer dereference.
>
> Fix rxrpc_input_ackall() to mirror the transmit-state gating already
> used for normal ACK processing, and ignore ACKALL when there is no
> outstanding transmit window to rotate.
>
> Fixes: b341a0263b1b ("rxrpc: Implement progressive transmission queue struct")
> Cc: stable@vger.kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Zhengchuan Liang <zcliangcn@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Assisted-by: Codex:GPT-5.4
> Signed-off-by: Wyatt Feng <bronzed_45_vested@icloud.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: linux-afs@lists.infradead.org
> ---
>   net/rxrpc/input.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index ce761466b02d..37881dffa898 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c
> @@ -1214,8 +1214,22 @@ static void rxrpc_input_ack(struct rxrpc_call *call, struct sk_buff *skb)
>   static void rxrpc_input_ackall(struct rxrpc_call *call, struct sk_buff *skb)
>   {
>   	struct rxrpc_ack_summary summary = { 0 };
> +	rxrpc_seq_t top = READ_ONCE(call->tx_top);
> +
> +	switch (__rxrpc_call_state(call)) {
> +	case RXRPC_CALL_CLIENT_SEND_REQUEST:
> +	case RXRPC_CALL_CLIENT_AWAIT_REPLY:
> +	case RXRPC_CALL_SERVER_SEND_REPLY:
> +	case RXRPC_CALL_SERVER_AWAIT_ACK:
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (call->tx_bottom == top)
> +		return;
>   
> -	if (rxrpc_rotate_tx_window(call, call->tx_top, &summary))
> +	if (rxrpc_rotate_tx_window(call, top, &summary))
>   		rxrpc_end_tx_phase(call, false, rxrpc_eproto_unexpected_ackall);
>   }
>   

Wyatt,

Thank you for identifying the NULL pointer dereference but I do not 
believe the patch is correct from an RxRPC protocol perspective.

The rxrpc protocol is not formally standardized.  Linux rxrpc is a clean 
room implementation of Transarc/IBM RxRPC protocol used by AFS 3.0.

I've been spelunking through old source code trees dating back to 
mid-1988.  The original usage of the ACKALL packet was a form of delayed 
acknowledgement only to be sent after all of the DATA packets inclusive 
of the LAST_PACKET had been received.  Your expectation of how the 
packet type is intended to be used is consistent with that behavior.

However, in Nov 1988 the DATA acknowledgement logic was altered in a 
backward incompatible manner.   Instead of immediately sending ACK 
packets in response to every DATA packet except when the final DATA 
packet inclusive of LAST_PACKET was received, the ACK packet usage was 
extended to permit delayed transmissions. From Nov 1988 onward ACK 
packets were scheduled to be sent with a 200ms delay unless the received 
DATA packet was a duplicate, out-of-sequence, out-of-window, etc OR 
unless the received DATA packet had the RX_REQUEST_ACK flag set.   The 
delayed ACKs replaced the ACKALL usage in the general case.

But it appears there was a bug introduced which resulted in the sending 
of arbitrary ACKALL packets at any point in the call lifetime.   This 
bug was not identified until Nov 2001 [OpenAFS 
db2ddfaf1b322710e1bd4edce6d7519157c3c9eb] at which point the sending of 
ACKALL packets was further restricted.  One of the reasons why the 
sending of ACKALL packets at arbitrary times was not identified as a 
problem for more than a decade is that ACKALL packets received when 
there were no transmitted packets waiting for acknowledgement had no 
impact on the call state.   If there were transmitted packets waiting 
for acknowledgement and they were successfully delivered, then the call 
continued successfully.

OpenAFS 1.6 pre-releases attempted to resume use of ACKALL packets as a 
performance enhancement only to revert the change because of 
compatibility problems.

I think the best change at this point would to accept the ACKALL packets 
without generating an error regardless of the call state. If there are 
transmitted DATA packets waiting for acknowledgement, acknowledge them.  
If there are DATA packets which have yet to be sent, leave them alone.  
Only complete the call in response to an ACKALL if the ACKALL is 
received by the acceptor (incoming call) and all DATA packets inclusive 
of LAST_PACKET have been transmitted at least once.

Sincerely,

Jeffrey Altman




  reply	other threads:[~2026-06-19 21:38 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-18 13:47 [PATCH net v2 00/10] rxrpc: Miscellaneous fixes David Howells
2026-06-18 13:47 ` [PATCH net v2 01/10] rxrpc: input: reject ACKALL outside transmit phase David Howells
2026-06-19 21:32   ` Jeffrey E Altman [this message]
2026-06-18 13:47 ` [PATCH net v2 02/10] rxrpc: Fix leak of connection from OOB challenge David Howells
2026-06-18 13:47 ` [PATCH net v2 03/10] rxrpc: Fix double unlock in rxrpc_recvmsg() David Howells
2026-06-18 13:47 ` [PATCH net v2 04/10] afs: Fix further netns teardown to cancel the preallocation charger David Howells
2026-06-18 13:47 ` [PATCH net v2 05/10] afs: Fix uncancelled rxrpc OOB message handler David Howells
2026-06-20  9:13   ` Simon Horman
2026-06-18 13:47 ` [PATCH net v2 06/10] rxrpc: Fix the reception of a reply packet before data transmission David Howells
2026-06-20  9:17   ` Simon Horman
2026-06-18 13:47 ` [PATCH net v2 07/10] rxrpc: Fix oob challenge leak in cleanup after notification failure David Howells
2026-06-20  9:17   ` Simon Horman
2026-06-18 13:47 ` [PATCH net v2 08/10] rxrpc: Fix potential infinite loop in rxrpc_recvmsg() David Howells
2026-06-18 13:48 ` [PATCH net v2 09/10] rxrpc: Fix socket notification race David Howells
2026-06-18 13:48 ` [PATCH net v2 10/10] rxrpc: Fix leak of released call in recvmsg(MSG_PEEK) David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c0fd4fec-1576-4070-b31e-a37d5506f5ed@auristor.com \
    --to=jaltman@auristor.com \
    --cc=bird@lzu.edu.cn \
    --cc=bronzed_45_vested@icloud.com \
    --cc=davem@davemloft.net \
    --cc=dhowells@redhat.com \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.dionne@auristor.com \
    --cc=n05ec@lzu.edu.cn \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=stable@vger.kernel.org \
    --cc=tomapufckgml@gmail.com \
    --cc=yifanwucs@gmail.com \
    --cc=yuantan098@gmail.com \
    --cc=zcliangcn@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox