| Message ID | 20230831165811.18061-1-edward.cree@amd.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ae074e2b2fd410bf54d56509a7e48fb83873af3b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sfc: check for zero length in EF10 RX prefix | expand |
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>
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!
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
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
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;