[PATCH net] sfc: check for zero length in EF10 RX prefix

[PATCH net] sfc: check for zero length in EF10 RX prefix

[edward.cree]

From: Edward Cree <ecree.xilinx@gmail.com>

When EF10 RXDP firmware is operating in cut-through mode, packet length
 is not known at the time the RX prefix is generated, so it is left as
 zero and RX event merging is inhibited to ensure that the length is
 available in the RX event.  However, it has been found that in certain
 circumstances the RX events for these packets still get merged,
 meaning the driver cannot read the length from the RX event, and tries
 to use the length from the prefix.
The resulting zero-length SKBs cause crashes in GRO since commit
 1d11fa696733 ("net-gro: remove GRO_DROP"), so add a check to the driver
 to detect these zero-length RX events and discard the packet.

Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/rx.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
index 2375cef577e4..f77a2d3ef37e 100644
--- a/drivers/net/ethernet/sfc/rx.c
+++ b/drivers/net/ethernet/sfc/rx.c
@@ -359,26 +359,36 @@ static bool efx_do_xdp(struct efx_nic *efx, struct efx_channel *channel,
 /* Handle a received packet.  Second half: Touches packet payload. */
 void __efx_rx_packet(struct efx_channel *channel)
 {
+	struct efx_rx_queue *rx_queue = efx_channel_get_rx_queue(channel);
 	struct efx_nic *efx = channel->efx;
 	struct efx_rx_buffer *rx_buf =
-		efx_rx_buffer(&channel->rx_queue, channel->rx_pkt_index);
+		efx_rx_buffer(rx_queue, channel->rx_pkt_index);
 	u8 *eh = efx_rx_buf_va(rx_buf);
 
 	/* Read length from the prefix if necessary.  This already
 	 * excludes the length of the prefix itself.
 	 */
-	if (rx_buf->flags & EFX_RX_PKT_PREFIX_LEN)
+	if (rx_buf->flags & EFX_RX_PKT_PREFIX_LEN) {
 		rx_buf->len = le16_to_cpup((__le16 *)
 					   (eh + efx->rx_packet_len_offset));
+		/* A known issue may prevent this being filled in;
+		 * if that happens, just drop the packet.
+		 * Must do that in the driver since passing a zero-length
+		 * packet up to the stack may cause a crash.
+		 */
+		if (unlikely(!rx_buf->len)) {
+			efx_free_rx_buffers(rx_queue, rx_buf,
+					    channel->rx_pkt_n_frags);
+			channel->n_rx_frm_trunc++;
+			goto out;
+		}
+	}
 
 	/* If we're in loopback test, then pass the packet directly to the
 	 * loopback layer, and free the rx_buf here
 	 */
 	if (unlikely(efx->loopback_selftest)) {
-		struct efx_rx_queue *rx_queue;
-
 		efx_loopback_rx_packet(efx, eh, rx_buf->len);
-		rx_queue = efx_channel_get_rx_queue(channel);
 		efx_free_rx_buffers(rx_queue, rx_buf,
 				    channel->rx_pkt_n_frags);
 		goto out;

Re: [PATCH net] sfc: check for zero length in EF10 RX prefix

[Eric Dumazet]

On Thu, Aug 31, 2023 at 6:59 PM <edward.cree@amd.com> wrote:
>
> From: Edward Cree <ecree.xilinx@gmail.com>
>
> When EF10 RXDP firmware is operating in cut-through mode, packet length
>  is not known at the time the RX prefix is generated, so it is left as
>  zero and RX event merging is inhibited to ensure that the length is
>  available in the RX event.  However, it has been found that in certain
>  circumstances the RX events for these packets still get merged,
>  meaning the driver cannot read the length from the RX event, and tries
>  to use the length from the prefix.
> The resulting zero-length SKBs cause crashes in GRO since commit
>  1d11fa696733 ("net-gro: remove GRO_DROP"), so add a check to the driver
>  to detect these zero-length RX events and discard the packet.
>
> Signed-off-by: Edward Cree <ecree.xilinx@gmail.com>

Nice bug ;)

Thanks for this fix.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] sfc: check for zero length in EF10 RX prefix

[patchwork-bot+netdevbpf]

Hello:

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

On Thu, 31 Aug 2023 17:58:11 +0100 you wrote:
> From: Edward Cree <ecree.xilinx@gmail.com>
> 
> When EF10 RXDP firmware is operating in cut-through mode, packet length
>  is not known at the time the RX prefix is generated, so it is left as
>  zero and RX event merging is inhibited to ensure that the length is
>  available in the RX event.  However, it has been found that in certain
>  circumstances the RX events for these packets still get merged,
>  meaning the driver cannot read the length from the RX event, and tries
>  to use the length from the prefix.
> The resulting zero-length SKBs cause crashes in GRO since commit
>  1d11fa696733 ("net-gro: remove GRO_DROP"), so add a check to the driver
>  to detect these zero-length RX events and discard the packet.
> 
> [...]

Here is the summary with links:
  - [net] sfc: check for zero length in EF10 RX prefix
    https://git.kernel.org/netdev/net/c/ae074e2b2fd4

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

Re: [PATCH net] sfc: check for zero length in EF10 RX prefix

[Jay Vosburgh]

patchwork-bot+netdevbpf@kernel.org wrote:

>Hello:
>
>This patch was applied to netdev/net.git (main)
>by David S. Miller <davem@davemloft.net>:
>
>On Thu, 31 Aug 2023 17:58:11 +0100 you wrote:
>> From: Edward Cree <ecree.xilinx@gmail.com>
>> 
>> When EF10 RXDP firmware is operating in cut-through mode, packet length
>>  is not known at the time the RX prefix is generated, so it is left as
>>  zero and RX event merging is inhibited to ensure that the length is
>>  available in the RX event.  However, it has been found that in certain
>>  circumstances the RX events for these packets still get merged,
>>  meaning the driver cannot read the length from the RX event, and tries
>>  to use the length from the prefix.
>> The resulting zero-length SKBs cause crashes in GRO since commit
>>  1d11fa696733 ("net-gro: remove GRO_DROP"), so add a check to the driver
>>  to detect these zero-length RX events and discard the packet.
>> 
>> [...]

	Should this have included

Fixes: 1d11fa696733 ("net-gro: remove GRO_DROP")

	to queue the patch for -stable?  We have users running into this
issue on 5.15 series kernels.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] sfc: check for zero length in EF10 RX prefix

[Edward Cree]

On 01/09/2023 21:28, Jay Vosburgh wrote:
>>> The resulting zero-length SKBs cause crashes in GRO since commit
>>>  1d11fa696733 ("net-gro: remove GRO_DROP"), so add a check to the driver
>>>  to detect these zero-length RX events and discard the packet.
>>>
>>> [...]
> 
> 	Should this have included
> 
> Fixes: 1d11fa696733 ("net-gro: remove GRO_DROP")
> 
> 	to queue the patch for -stable?  We have users running into this
> issue on 5.15 series kernels.

I didn't think Fixes: was appropriate (for various abstruse reasons I
 won't go into), but this does want to go to -stable.  I expect Sasha's
 robot will select it, but feel free to submit it explicitly.

-ed