Skip to content

Commit d760117

Browse files
davidarinzonkuba-moo
authored andcommitted
net: ena: Fix DMA syncing in XDP path when SWIOTLB is on
This patch fixes two issues: Issue 1 ------- Description ``````````` Current code does not call dma_sync_single_for_cpu() to sync data from the device side memory to the CPU side memory before the XDP code path uses the CPU side data. This causes the XDP code path to read the unset garbage data in the CPU side memory, resulting in incorrect handling of the packet by XDP. Solution ```````` 1. Add a call to dma_sync_single_for_cpu() before the XDP code starts to use the data in the CPU side memory. 2. The XDP code verdict can be XDP_PASS, in which case there is a fallback to the non-XDP code, which also calls dma_sync_single_for_cpu(). To avoid calling dma_sync_single_for_cpu() twice: 2.1. Put the dma_sync_single_for_cpu() in the code in such a place where it happens before XDP and non-XDP code. 2.2. Remove the calls to dma_sync_single_for_cpu() in the non-XDP code for the first buffer only (rx_copybreak and non-rx_copybreak cases), since the new call that was added covers these cases. The call to dma_sync_single_for_cpu() for the second buffer and on stays because only the first buffer is handled by the newly added dma_sync_single_for_cpu(). And there is no need for special handling of the second buffer and on for the XDP path since currently the driver supports only single buffer packets. Issue 2 ------- Description ``````````` In case the XDP code forwarded the packet (ENA_XDP_FORWARDED), ena_unmap_rx_buff_attrs() is called with attrs set to 0. This means that before unmapping the buffer, the internal function dma_unmap_page_attrs() will also call dma_sync_single_for_cpu() on the whole buffer (not only on the data part of it). This sync is both wasteful (since a sync was already explicitly called before) and also causes a bug, which will be explained using the below diagram. The following diagram shows the flow of events causing the bug. The order of events is (1)-(4) as shown in the diagram. CPU side memory area (3)convert_to_xdp_frame() initializes the headroom with xdpf metadata || \/ ___________________________________ | | 0 | V 4K --------------------------------------------------------------------- | xdpf->data | other xdpf | < data > | tailroom ||...| | | fields | | GARBAGE || | --------------------------------------------------------------------- /\ /\ || || (4)ena_unmap_rx_buff_attrs() calls (2)dma_sync_single_for_cpu() dma_sync_single_for_cpu() on the copies data from device whole buffer page, overwriting side to CPU side memory the xdpf->data with GARBAGE. || 0 4K --------------------------------------------------------------------- | headroom | < data > | tailroom ||...| | GARBAGE | | GARBAGE || | --------------------------------------------------------------------- Device side memory area /\ || (1) device writes RX packet data After the call to ena_unmap_rx_buff_attrs() in (4), the xdpf->data becomes corrupted, and so when it is later accessed in ena_clean_xdp_irq()->xdp_return_frame(), it causes a page fault, crashing the kernel. Solution ```````` Explicitly tell ena_unmap_rx_buff_attrs() not to call dma_sync_single_for_cpu() by passing it the ENA_DMA_ATTR_SKIP_CPU_SYNC flag. Fixes: f7d625a ("net: ena: Add dynamic recycling mechanism for rx buffers") Signed-off-by: Arthur Kiyanovski <[email protected]> Signed-off-by: David Arinzon <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>
1 parent 505b1a8 commit d760117

File tree

1 file changed

+9
-14
lines changed

1 file changed

+9
-14
lines changed

drivers/net/ethernet/amazon/ena/ena_netdev.c

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1493,11 +1493,6 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
14931493
if (unlikely(!skb))
14941494
return NULL;
14951495

1496-
/* sync this buffer for CPU use */
1497-
dma_sync_single_for_cpu(rx_ring->dev,
1498-
dma_unmap_addr(&rx_info->ena_buf, paddr) + pkt_offset,
1499-
len,
1500-
DMA_FROM_DEVICE);
15011496
skb_copy_to_linear_data(skb, buf_addr + buf_offset, len);
15021497
dma_sync_single_for_device(rx_ring->dev,
15031498
dma_unmap_addr(&rx_info->ena_buf, paddr) + pkt_offset,
@@ -1516,17 +1511,10 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
15161511

15171512
buf_len = SKB_DATA_ALIGN(len + buf_offset + tailroom);
15181513

1519-
pre_reuse_paddr = dma_unmap_addr(&rx_info->ena_buf, paddr);
1520-
15211514
/* If XDP isn't loaded try to reuse part of the RX buffer */
15221515
reuse_rx_buf_page = !is_xdp_loaded &&
15231516
ena_try_rx_buf_page_reuse(rx_info, buf_len, len, pkt_offset);
15241517

1525-
dma_sync_single_for_cpu(rx_ring->dev,
1526-
pre_reuse_paddr + pkt_offset,
1527-
len,
1528-
DMA_FROM_DEVICE);
1529-
15301518
if (!reuse_rx_buf_page)
15311519
ena_unmap_rx_buff_attrs(rx_ring, rx_info, DMA_ATTR_SKIP_CPU_SYNC);
15321520

@@ -1723,6 +1711,7 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
17231711
int xdp_flags = 0;
17241712
int total_len = 0;
17251713
int xdp_verdict;
1714+
u8 pkt_offset;
17261715
int rc = 0;
17271716
int i;
17281717

@@ -1749,13 +1738,19 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
17491738

17501739
/* First descriptor might have an offset set by the device */
17511740
rx_info = &rx_ring->rx_buffer_info[rx_ring->ena_bufs[0].req_id];
1752-
rx_info->buf_offset += ena_rx_ctx.pkt_offset;
1741+
pkt_offset = ena_rx_ctx.pkt_offset;
1742+
rx_info->buf_offset += pkt_offset;
17531743

17541744
netif_dbg(rx_ring->adapter, rx_status, rx_ring->netdev,
17551745
"rx_poll: q %d got packet from ena. descs #: %d l3 proto %d l4 proto %d hash: %x\n",
17561746
rx_ring->qid, ena_rx_ctx.descs, ena_rx_ctx.l3_proto,
17571747
ena_rx_ctx.l4_proto, ena_rx_ctx.hash);
17581748

1749+
dma_sync_single_for_cpu(rx_ring->dev,
1750+
dma_unmap_addr(&rx_info->ena_buf, paddr) + pkt_offset,
1751+
rx_ring->ena_bufs[0].len,
1752+
DMA_FROM_DEVICE);
1753+
17591754
if (ena_xdp_present_ring(rx_ring))
17601755
xdp_verdict = ena_xdp_handle_buff(rx_ring, &xdp, ena_rx_ctx.descs);
17611756

@@ -1781,7 +1776,7 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
17811776
if (xdp_verdict & ENA_XDP_FORWARDED) {
17821777
ena_unmap_rx_buff_attrs(rx_ring,
17831778
&rx_ring->rx_buffer_info[req_id],
1784-
0);
1779+
DMA_ATTR_SKIP_CPU_SYNC);
17851780
rx_ring->rx_buffer_info[req_id].page = NULL;
17861781
}
17871782
}

0 commit comments

Comments
 (0)