| Message ID | 20240618041412.3184919-1-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ionic: fix kernel panic due to multi-buffer handling | expand |
On 6/17/2024 9:14 PM, Taehee Yoo wrote: > > Currently, the ionic_run_xdp() doesn't handle multi-buffer packets > properly for XDP_TX and XDP_REDIRECT. > When a jumbo frame is received, the ionic_run_xdp() first makes xdp > frame with all necessary pages in the rx descriptor. > And if the action is either XDP_TX or XDP_REDIRECT, it should unmap > dma-mapping and reset page pointer to NULL for all pages, not only the > first page. > But it doesn't for SG pages. So, SG pages unexpectedly will be reused. > It eventually causes kernel panic. > > Oops: general protection fault, probably for non-canonical address 0x504f4e4dbebc64ff: 0000 [#1] PREEMPT SMP NOPTI > CPU: 3 PID: 0 Comm: swapper/3 Not tainted 6.10.0-rc3+ #25 > RIP: 0010:xdp_return_frame+0x42/0x90 > Code: 01 75 12 5b 4c 89 e6 5d 31 c9 41 5c 31 d2 41 5d e9 73 fd ff ff 44 8b 6b 20 0f b7 43 0a 49 81 ed 68 01 00 00 49 29 c5 49 01 fd <41> 80 7d0 > RSP: 0018:ffff99d00122ce08 EFLAGS: 00010202 > RAX: 0000000000005453 RBX: ffff8d325f904000 RCX: 0000000000000001 > RDX: 00000000670e1000 RSI: 000000011f90d000 RDI: 504f4e4d4c4b4a49 > RBP: ffff99d003907740 R08: 0000000000000000 R09: 0000000000000000 > R10: 000000011f90d000 R11: 0000000000000000 R12: ffff8d325f904010 > R13: 504f4e4dbebc64fd R14: ffff8d3242b070c8 R15: ffff99d0039077c0 > FS: 0000000000000000(0000) GS:ffff8d399f780000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f41f6c85e38 CR3: 000000037ac30000 CR4: 00000000007506f0 > PKRU: 55555554 > Call Trace: > <IRQ> > ? die_addr+0x33/0x90 > ? exc_general_protection+0x251/0x2f0 > ? asm_exc_general_protection+0x22/0x30 > ? xdp_return_frame+0x42/0x90 > ionic_tx_clean+0x211/0x280 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] > ionic_tx_cq_service+0xd3/0x210 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] > ionic_txrx_napi+0x41/0x1b0 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] > __napi_poll.constprop.0+0x29/0x1b0 > net_rx_action+0x2c4/0x350 > handle_softirqs+0xf4/0x320 > irq_exit_rcu+0x78/0xa0 > common_interrupt+0x77/0x90 > > Fixes: 5377805dc1c0 ("ionic: implement xdp frags support") > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > --- > > In the XDP_DROP and XDP_ABORTED path, the ionic_rx_page_free() is called > to free page and unmap dma-mapping. > It resets only the first page pointer to NULL. > DROP/ABORTED path looks like it also has the same problem, but it's not. > Because all pages in the XDP_DROP/ABORTED path are not used anywhere it > can be reused without any free and unmap. > So, it's okay to remove the function in the xdp path. > > I will send a separated patch for removing ionic_rx_page_free() in the > xdp path. > > .../net/ethernet/pensando/ionic/ionic_txrx.c | 26 +++++++++++++++---- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c > index 2427610f4306..792e530e3281 100644 > --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c > +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c > @@ -487,14 +487,13 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > struct ionic_buf_info *buf_info, > int len) > { > + int remain_len, frag_len, i, err = 0; > + struct skb_shared_info *sinfo; > u32 xdp_action = XDP_ABORTED; > struct xdp_buff xdp_buf; > struct ionic_queue *txq; > struct netdev_queue *nq; > struct xdp_frame *xdpf; > - int remain_len; > - int frag_len; > - int err = 0; There's no need to change these 3 declarations. > > xdp_init_buff(&xdp_buf, IONIC_PAGE_SIZE, rxq->xdp_rxq_info); > frag_len = min_t(u16, len, IONIC_XDP_MAX_LINEAR_MTU + VLAN_ETH_HLEN); > @@ -513,7 +512,6 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > */ > remain_len = len - frag_len; > if (remain_len) { > - struct skb_shared_info *sinfo; > struct ionic_buf_info *bi; > skb_frag_t *frag; > > @@ -576,7 +574,6 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > > dma_unmap_page(rxq->dev, buf_info->dma_addr, > IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > - You can leave the whitespace alone > err = ionic_xdp_post_frame(txq, xdpf, XDP_TX, > buf_info->page, > buf_info->page_offset, > @@ -587,12 +584,22 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > goto out_xdp_abort; > } > buf_info->page = NULL; > + if (xdp_frame_has_frags(xdpf)) { You could use xdp_buff_has_frags(&xdp_buf) instead, or just drop this check and let nr_frags value handle it in the for-loop test, as long as you init sinfo->nr_frags = 0 at the start. > + for (i = 0; i < sinfo->nr_frags; i++) { > + buf_info++; > + dma_unmap_page(rxq->dev, buf_info->dma_addr, > + IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > + buf_info->page = NULL; > + } > + } > + > stats->xdp_tx++; > > /* the Tx completion will free the buffers */ > break; > > case XDP_REDIRECT: > + xdpf = xdp_convert_buff_to_frame(&xdp_buf); > /* unmap the pages before handing them to a different device */ > dma_unmap_page(rxq->dev, buf_info->dma_addr, > IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > @@ -603,6 +610,15 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > goto out_xdp_abort; > } > buf_info->page = NULL; > + if (xdp_frame_has_frags(xdpf)) { If you use xdp_buff_has_frags() then you would not have to waste time with xdp_convert_buff_to_frame(). Or, again, if you init nr_flags at the start then you could skip the test altogether and let the for-loop test deal with it. > + for (i = 0; i < sinfo->nr_frags; i++) { > + buf_info++; > + dma_unmap_page(rxq->dev, buf_info->dma_addr, > + IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > + buf_info->page = NULL; > + } > + } > + Since this is repeated code, maybe build a little function, something like static void ionic_xdp_rx_put_frags(struct ionic_queue *q, struct ionic_buf_info *buf_info, int nfrags) { int i; for (i = 0; i < nfrags; i++) { buf_info++; dma_unmap_page(rxq->dev, buf_info->dma_addr, IONIC_PAGE_SIZE, DMA_FROM_DEVICE); buf_info->page = NULL; } } and call this with ionic_xdp_rx_put_frags(rxq, buf_info, sinfo->nr_frags); Meanwhile, before removing ionic_rx_page_free(), be sure to check that ionic_rx_fill() notices that there are useful pages already there that can be reused and doesn't end up leaking pages. We have some draft patches for converting this all to page_pool, which I think takes care of this and some other messy bits. We've been side tracked internally, but really need to get back to those. Thanks, sln > rxq->xdp_flush = true; > stats->xdp_redirect++; > break; > -- > 2.34.1 >
On Wed, Jun 19, 2024 at 2:40 AM Nelson, Shannon <shannon.nelson@amd.com> wrote: Hi Nelson, Shannon, Thank you for the review! > > On 6/17/2024 9:14 PM, Taehee Yoo wrote: > > > > Currently, the ionic_run_xdp() doesn't handle multi-buffer packets > > properly for XDP_TX and XDP_REDIRECT. > > When a jumbo frame is received, the ionic_run_xdp() first makes xdp > > frame with all necessary pages in the rx descriptor. > > And if the action is either XDP_TX or XDP_REDIRECT, it should unmap > > dma-mapping and reset page pointer to NULL for all pages, not only the > > first page. > > But it doesn't for SG pages. So, SG pages unexpectedly will be reused. > > It eventually causes kernel panic. > > > > Oops: general protection fault, probably for non-canonical address 0x504f4e4dbebc64ff: 0000 [#1] PREEMPT SMP NOPTI > > CPU: 3 PID: 0 Comm: swapper/3 Not tainted 6.10.0-rc3+ #25 > > RIP: 0010:xdp_return_frame+0x42/0x90 > > Code: 01 75 12 5b 4c 89 e6 5d 31 c9 41 5c 31 d2 41 5d e9 73 fd ff ff 44 8b 6b 20 0f b7 43 0a 49 81 ed 68 01 00 00 49 29 c5 49 01 fd <41> 80 7d0 > > RSP: 0018:ffff99d00122ce08 EFLAGS: 00010202 > > RAX: 0000000000005453 RBX: ffff8d325f904000 RCX: 0000000000000001 > > RDX: 00000000670e1000 RSI: 000000011f90d000 RDI: 504f4e4d4c4b4a49 > > RBP: ffff99d003907740 R08: 0000000000000000 R09: 0000000000000000 > > R10: 000000011f90d000 R11: 0000000000000000 R12: ffff8d325f904010 > > R13: 504f4e4dbebc64fd R14: ffff8d3242b070c8 R15: ffff99d0039077c0 > > FS: 0000000000000000(0000) GS:ffff8d399f780000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 00007f41f6c85e38 CR3: 000000037ac30000 CR4: 00000000007506f0 > > PKRU: 55555554 > > Call Trace: > > <IRQ> > > ? die_addr+0x33/0x90 > > ? exc_general_protection+0x251/0x2f0 > > ? asm_exc_general_protection+0x22/0x30 > > ? xdp_return_frame+0x42/0x90 > > ionic_tx_clean+0x211/0x280 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] > > ionic_tx_cq_service+0xd3/0x210 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] > > ionic_txrx_napi+0x41/0x1b0 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] > > __napi_poll.constprop.0+0x29/0x1b0 > > net_rx_action+0x2c4/0x350 > > handle_softirqs+0xf4/0x320 > > irq_exit_rcu+0x78/0xa0 > > common_interrupt+0x77/0x90 > > > > Fixes: 5377805dc1c0 ("ionic: implement xdp frags support") > > Signed-off-by: Taehee Yoo <ap420073@gmail.com> > > --- > > > > In the XDP_DROP and XDP_ABORTED path, the ionic_rx_page_free() is called > > to free page and unmap dma-mapping. > > It resets only the first page pointer to NULL. > > DROP/ABORTED path looks like it also has the same problem, but it's not. > > Because all pages in the XDP_DROP/ABORTED path are not used anywhere it > > can be reused without any free and unmap. > > So, it's okay to remove the function in the xdp path. > > > > I will send a separated patch for removing ionic_rx_page_free() in the > > xdp path. > > > > .../net/ethernet/pensando/ionic/ionic_txrx.c | 26 +++++++++++++++---- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c > > index 2427610f4306..792e530e3281 100644 > > --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c > > +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c > > @@ -487,14 +487,13 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > > struct ionic_buf_info *buf_info, > > int len) > > { > > + int remain_len, frag_len, i, err = 0; > > + struct skb_shared_info *sinfo; > > u32 xdp_action = XDP_ABORTED; > > struct xdp_buff xdp_buf; > > struct ionic_queue *txq; > > struct netdev_queue *nq; > > struct xdp_frame *xdpf; > > - int remain_len; > > - int frag_len; > > - int err = 0; > > There's no need to change these 3 declarations. Okay, I will add a new declaration 'i', not change these declarations. > > > > > xdp_init_buff(&xdp_buf, IONIC_PAGE_SIZE, rxq->xdp_rxq_info); > > frag_len = min_t(u16, len, IONIC_XDP_MAX_LINEAR_MTU + VLAN_ETH_HLEN); > > @@ -513,7 +512,6 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > > */ > > remain_len = len - frag_len; > > if (remain_len) { > > - struct skb_shared_info *sinfo; > > struct ionic_buf_info *bi; > > skb_frag_t *frag; > > > > @@ -576,7 +574,6 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > > > > dma_unmap_page(rxq->dev, buf_info->dma_addr, > > IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > > - > > You can leave the whitespace alone Thanks, I will remove it :) > > > err = ionic_xdp_post_frame(txq, xdpf, XDP_TX, > > buf_info->page, > > buf_info->page_offset, > > @@ -587,12 +584,22 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > > goto out_xdp_abort; > > } > > buf_info->page = NULL; > > + if (xdp_frame_has_frags(xdpf)) { > > You could use xdp_buff_has_frags(&xdp_buf) instead, or just drop this > check and let nr_frags value handle it in the for-loop test, as long as > you init sinfo->nr_frags = 0 at the start. Right, xdp_buff_has_frags() is not needed here indeed. I will remove it, Thanks! > > > > + for (i = 0; i < sinfo->nr_frags; i++) { > > + buf_info++; > > + dma_unmap_page(rxq->dev, buf_info->dma_addr, > > + IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > > + buf_info->page = NULL; > > + } > > + } > > + > > stats->xdp_tx++; > > > > /* the Tx completion will free the buffers */ > > break; > > > > case XDP_REDIRECT: > > + xdpf = xdp_convert_buff_to_frame(&xdp_buf); > > /* unmap the pages before handing them to a different device */ > > dma_unmap_page(rxq->dev, buf_info->dma_addr, > > IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > > @@ -603,6 +610,15 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, > > goto out_xdp_abort; > > } > > buf_info->page = NULL; > > + if (xdp_frame_has_frags(xdpf)) { > > If you use xdp_buff_has_frags() then you would not have to waste time > with xdp_convert_buff_to_frame(). Or, again, if you init nr_flags at > the start then you could skip the test altogether and let the for-loop > test deal with it. > > > + for (i = 0; i < sinfo->nr_frags; i++) { > > + buf_info++; > > + dma_unmap_page(rxq->dev, buf_info->dma_addr, > > + IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > > + buf_info->page = NULL; > > + } > > + } > > + > > Since this is repeated code, maybe build a little function, something like > > static void ionic_xdp_rx_put_frags(struct ionic_queue *q, > struct ionic_buf_info *buf_info, > int nfrags) > { > int i; > > for (i = 0; i < nfrags; i++) { > buf_info++; > dma_unmap_page(rxq->dev, buf_info->dma_addr, > IONIC_PAGE_SIZE, DMA_FROM_DEVICE); > buf_info->page = NULL; > } > } > > and call this with > ionic_xdp_rx_put_frags(rxq, buf_info, sinfo->nr_frags); It looks great, I will apply your suggestion. I will send a v2 patch with the above change :) > > > Meanwhile, before removing ionic_rx_page_free(), be sure to check that > ionic_rx_fill() notices that there are useful pages already there that > can be reused and doesn't end up leaking pages. > > We have some draft patches for converting this all to page_pool, which I > think takes care of this and some other messy bits. We've been side > tracked internally, but really need to get back to those. > Oh, I also planned to send a patch for converting page_pool. And at that patch, ionic_rx_page_free() was going to be removed. But you're already preparing to send a patch for converting to page_pool, so I will not send my page_pool patch. And I will test your page_pool patch. I agree with the page_pool approach because I checked the improvement performance and it makes the code a little bit simpler. Thanks a lot for sharing your plan :) > Thanks, > sln > > > rxq->xdp_flush = true; > > stats->xdp_redirect++; > > break; > > -- > > 2.34.1 > > Thanks a lot! Taehee Yoo
Hi Taehee,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Taehee-Yoo/ionic-fix-kernel-panic-due-to-multi-buffer-handling/20240618-121629
base: net/main
patch link: https://lore.kernel.org/r/20240618041412.3184919-1-ap420073%40gmail.com
patch subject: [PATCH net] ionic: fix kernel panic due to multi-buffer handling
config: x86_64-randconfig-161-20240620 (https://download.01.org/0day-ci/archive/20240621/202406211040.17BtI0lu-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202406211040.17BtI0lu-lkp@intel.com/
New smatch warnings:
drivers/net/ethernet/pensando/ionic/ionic_txrx.c:588 ionic_run_xdp() error: uninitialized symbol 'sinfo'.
Old smatch warnings:
drivers/net/ethernet/pensando/ionic/ionic_txrx.c:253 ionic_rx_build_skb() warn: variable dereferenced before check 'buf_info->page' (see line 237)
drivers/net/ethernet/pensando/ionic/ionic_txrx.c:614 ionic_run_xdp() error: uninitialized symbol 'sinfo'.
vim +/sinfo +588 drivers/net/ethernet/pensando/ionic/ionic_txrx.c
180e35cdf035d1 Shannon Nelson 2024-02-14 483 static bool ionic_run_xdp(struct ionic_rx_stats *stats,
180e35cdf035d1 Shannon Nelson 2024-02-14 484 struct net_device *netdev,
180e35cdf035d1 Shannon Nelson 2024-02-14 485 struct bpf_prog *xdp_prog,
180e35cdf035d1 Shannon Nelson 2024-02-14 486 struct ionic_queue *rxq,
180e35cdf035d1 Shannon Nelson 2024-02-14 487 struct ionic_buf_info *buf_info,
180e35cdf035d1 Shannon Nelson 2024-02-14 488 int len)
180e35cdf035d1 Shannon Nelson 2024-02-14 489 {
17cb07b96a5f58 Taehee Yoo 2024-06-18 490 int remain_len, frag_len, i, err = 0;
17cb07b96a5f58 Taehee Yoo 2024-06-18 491 struct skb_shared_info *sinfo;
180e35cdf035d1 Shannon Nelson 2024-02-14 492 u32 xdp_action = XDP_ABORTED;
180e35cdf035d1 Shannon Nelson 2024-02-14 493 struct xdp_buff xdp_buf;
8eeed8373e1cca Shannon Nelson 2024-02-14 494 struct ionic_queue *txq;
8eeed8373e1cca Shannon Nelson 2024-02-14 495 struct netdev_queue *nq;
8eeed8373e1cca Shannon Nelson 2024-02-14 496 struct xdp_frame *xdpf;
180e35cdf035d1 Shannon Nelson 2024-02-14 497
180e35cdf035d1 Shannon Nelson 2024-02-14 498 xdp_init_buff(&xdp_buf, IONIC_PAGE_SIZE, rxq->xdp_rxq_info);
5377805dc1c02a Shannon Nelson 2024-02-14 499 frag_len = min_t(u16, len, IONIC_XDP_MAX_LINEAR_MTU + VLAN_ETH_HLEN);
180e35cdf035d1 Shannon Nelson 2024-02-14 500 xdp_prepare_buff(&xdp_buf, ionic_rx_buf_va(buf_info),
5377805dc1c02a Shannon Nelson 2024-02-14 501 XDP_PACKET_HEADROOM, frag_len, false);
180e35cdf035d1 Shannon Nelson 2024-02-14 502
180e35cdf035d1 Shannon Nelson 2024-02-14 503 dma_sync_single_range_for_cpu(rxq->dev, ionic_rx_buf_pa(buf_info),
f81da39bf4c0a5 Shannon Nelson 2024-02-14 504 XDP_PACKET_HEADROOM, len,
180e35cdf035d1 Shannon Nelson 2024-02-14 505 DMA_FROM_DEVICE);
180e35cdf035d1 Shannon Nelson 2024-02-14 506
180e35cdf035d1 Shannon Nelson 2024-02-14 507 prefetchw(&xdp_buf.data_hard_start);
180e35cdf035d1 Shannon Nelson 2024-02-14 508
5377805dc1c02a Shannon Nelson 2024-02-14 509 /* We limit MTU size to one buffer if !xdp_has_frags, so
5377805dc1c02a Shannon Nelson 2024-02-14 510 * if the recv len is bigger than one buffer
5377805dc1c02a Shannon Nelson 2024-02-14 511 * then we know we have frag info to gather
5377805dc1c02a Shannon Nelson 2024-02-14 512 */
5377805dc1c02a Shannon Nelson 2024-02-14 513 remain_len = len - frag_len;
5377805dc1c02a Shannon Nelson 2024-02-14 514 if (remain_len) {
5377805dc1c02a Shannon Nelson 2024-02-14 515 struct ionic_buf_info *bi;
5377805dc1c02a Shannon Nelson 2024-02-14 516 skb_frag_t *frag;
5377805dc1c02a Shannon Nelson 2024-02-14 517
5377805dc1c02a Shannon Nelson 2024-02-14 518 bi = buf_info;
5377805dc1c02a Shannon Nelson 2024-02-14 519 sinfo = xdp_get_shared_info_from_buff(&xdp_buf);
5377805dc1c02a Shannon Nelson 2024-02-14 520 sinfo->nr_frags = 0;
5377805dc1c02a Shannon Nelson 2024-02-14 521 sinfo->xdp_frags_size = 0;
5377805dc1c02a Shannon Nelson 2024-02-14 522 xdp_buff_set_frags_flag(&xdp_buf);
5377805dc1c02a Shannon Nelson 2024-02-14 523
5377805dc1c02a Shannon Nelson 2024-02-14 524 do {
5377805dc1c02a Shannon Nelson 2024-02-14 525 if (unlikely(sinfo->nr_frags >= MAX_SKB_FRAGS)) {
5377805dc1c02a Shannon Nelson 2024-02-14 526 err = -ENOSPC;
5377805dc1c02a Shannon Nelson 2024-02-14 527 goto out_xdp_abort;
5377805dc1c02a Shannon Nelson 2024-02-14 528 }
5377805dc1c02a Shannon Nelson 2024-02-14 529
5377805dc1c02a Shannon Nelson 2024-02-14 530 frag = &sinfo->frags[sinfo->nr_frags];
5377805dc1c02a Shannon Nelson 2024-02-14 531 sinfo->nr_frags++;
5377805dc1c02a Shannon Nelson 2024-02-14 532 bi++;
5377805dc1c02a Shannon Nelson 2024-02-14 533 frag_len = min_t(u16, remain_len, ionic_rx_buf_size(bi));
5377805dc1c02a Shannon Nelson 2024-02-14 534 dma_sync_single_range_for_cpu(rxq->dev, ionic_rx_buf_pa(bi),
5377805dc1c02a Shannon Nelson 2024-02-14 535 0, frag_len, DMA_FROM_DEVICE);
5377805dc1c02a Shannon Nelson 2024-02-14 536 skb_frag_fill_page_desc(frag, bi->page, 0, frag_len);
5377805dc1c02a Shannon Nelson 2024-02-14 537 sinfo->xdp_frags_size += frag_len;
5377805dc1c02a Shannon Nelson 2024-02-14 538 remain_len -= frag_len;
5377805dc1c02a Shannon Nelson 2024-02-14 539
5377805dc1c02a Shannon Nelson 2024-02-14 540 if (page_is_pfmemalloc(bi->page))
5377805dc1c02a Shannon Nelson 2024-02-14 541 xdp_buff_set_frag_pfmemalloc(&xdp_buf);
5377805dc1c02a Shannon Nelson 2024-02-14 542 } while (remain_len > 0);
5377805dc1c02a Shannon Nelson 2024-02-14 543 }
sinfo uninitialized on else path.
5377805dc1c02a Shannon Nelson 2024-02-14 544
180e35cdf035d1 Shannon Nelson 2024-02-14 545 xdp_action = bpf_prog_run_xdp(xdp_prog, &xdp_buf);
180e35cdf035d1 Shannon Nelson 2024-02-14 546
180e35cdf035d1 Shannon Nelson 2024-02-14 547 switch (xdp_action) {
180e35cdf035d1 Shannon Nelson 2024-02-14 548 case XDP_PASS:
180e35cdf035d1 Shannon Nelson 2024-02-14 549 stats->xdp_pass++;
180e35cdf035d1 Shannon Nelson 2024-02-14 550 return false; /* false = we didn't consume the packet */
180e35cdf035d1 Shannon Nelson 2024-02-14 551
180e35cdf035d1 Shannon Nelson 2024-02-14 552 case XDP_DROP:
180e35cdf035d1 Shannon Nelson 2024-02-14 553 ionic_rx_page_free(rxq, buf_info);
180e35cdf035d1 Shannon Nelson 2024-02-14 554 stats->xdp_drop++;
180e35cdf035d1 Shannon Nelson 2024-02-14 555 break;
180e35cdf035d1 Shannon Nelson 2024-02-14 556
180e35cdf035d1 Shannon Nelson 2024-02-14 557 case XDP_TX:
8eeed8373e1cca Shannon Nelson 2024-02-14 558 xdpf = xdp_convert_buff_to_frame(&xdp_buf);
8eeed8373e1cca Shannon Nelson 2024-02-14 559 if (!xdpf)
8eeed8373e1cca Shannon Nelson 2024-02-14 560 goto out_xdp_abort;
8eeed8373e1cca Shannon Nelson 2024-02-14 561
8eeed8373e1cca Shannon Nelson 2024-02-14 562 txq = rxq->partner;
8eeed8373e1cca Shannon Nelson 2024-02-14 563 nq = netdev_get_tx_queue(netdev, txq->index);
8eeed8373e1cca Shannon Nelson 2024-02-14 564 __netif_tx_lock(nq, smp_processor_id());
8eeed8373e1cca Shannon Nelson 2024-02-14 565 txq_trans_cond_update(nq);
8eeed8373e1cca Shannon Nelson 2024-02-14 566
8eeed8373e1cca Shannon Nelson 2024-02-14 567 if (netif_tx_queue_stopped(nq) ||
1937b7ab6bd6bd Brett Creeley 2024-02-29 568 !netif_txq_maybe_stop(q_to_ndq(netdev, txq),
061b9bedbef124 Brett Creeley 2024-02-29 569 ionic_q_space_avail(txq),
061b9bedbef124 Brett Creeley 2024-02-29 570 1, 1)) {
8eeed8373e1cca Shannon Nelson 2024-02-14 571 __netif_tx_unlock(nq);
8eeed8373e1cca Shannon Nelson 2024-02-14 572 goto out_xdp_abort;
8eeed8373e1cca Shannon Nelson 2024-02-14 573 }
8eeed8373e1cca Shannon Nelson 2024-02-14 574
8eeed8373e1cca Shannon Nelson 2024-02-14 575 dma_unmap_page(rxq->dev, buf_info->dma_addr,
8eeed8373e1cca Shannon Nelson 2024-02-14 576 IONIC_PAGE_SIZE, DMA_FROM_DEVICE);
1937b7ab6bd6bd Brett Creeley 2024-02-29 577 err = ionic_xdp_post_frame(txq, xdpf, XDP_TX,
8eeed8373e1cca Shannon Nelson 2024-02-14 578 buf_info->page,
8eeed8373e1cca Shannon Nelson 2024-02-14 579 buf_info->page_offset,
8eeed8373e1cca Shannon Nelson 2024-02-14 580 true);
8eeed8373e1cca Shannon Nelson 2024-02-14 581 __netif_tx_unlock(nq);
8eeed8373e1cca Shannon Nelson 2024-02-14 582 if (err) {
8eeed8373e1cca Shannon Nelson 2024-02-14 583 netdev_dbg(netdev, "tx ionic_xdp_post_frame err %d\n", err);
8eeed8373e1cca Shannon Nelson 2024-02-14 584 goto out_xdp_abort;
8eeed8373e1cca Shannon Nelson 2024-02-14 585 }
491aee894a08bc Taehee Yoo 2024-06-03 586 buf_info->page = NULL;
17cb07b96a5f58 Taehee Yoo 2024-06-18 587 if (xdp_frame_has_frags(xdpf)) {
17cb07b96a5f58 Taehee Yoo 2024-06-18 @588 for (i = 0; i < sinfo->nr_frags; i++) {
^^^^^
Warning generated here
17cb07b96a5f58 Taehee Yoo 2024-06-18 589 buf_info++;
17cb07b96a5f58 Taehee Yoo 2024-06-18 590 dma_unmap_page(rxq->dev, buf_info->dma_addr,
17cb07b96a5f58 Taehee Yoo 2024-06-18 591 IONIC_PAGE_SIZE, DMA_FROM_DEVICE);
17cb07b96a5f58 Taehee Yoo 2024-06-18 592 buf_info->page = NULL;
17cb07b96a5f58 Taehee Yoo 2024-06-18 593 }
17cb07b96a5f58 Taehee Yoo 2024-06-18 594 }
17cb07b96a5f58 Taehee Yoo 2024-06-18 595
8eeed8373e1cca Shannon Nelson 2024-02-14 596 stats->xdp_tx++;
8eeed8373e1cca Shannon Nelson 2024-02-14 597
8eeed8373e1cca Shannon Nelson 2024-02-14 598 /* the Tx completion will free the buffers */
8eeed8373e1cca Shannon Nelson 2024-02-14 599 break;
8eeed8373e1cca Shannon Nelson 2024-02-14 600
180e35cdf035d1 Shannon Nelson 2024-02-14 601 case XDP_REDIRECT:
17cb07b96a5f58 Taehee Yoo 2024-06-18 602 xdpf = xdp_convert_buff_to_frame(&xdp_buf);
587fc3f0dceb08 Shannon Nelson 2024-02-14 603 /* unmap the pages before handing them to a different device */
587fc3f0dceb08 Shannon Nelson 2024-02-14 604 dma_unmap_page(rxq->dev, buf_info->dma_addr,
587fc3f0dceb08 Shannon Nelson 2024-02-14 605 IONIC_PAGE_SIZE, DMA_FROM_DEVICE);
587fc3f0dceb08 Shannon Nelson 2024-02-14 606
587fc3f0dceb08 Shannon Nelson 2024-02-14 607 err = xdp_do_redirect(netdev, &xdp_buf, xdp_prog);
587fc3f0dceb08 Shannon Nelson 2024-02-14 608 if (err) {
587fc3f0dceb08 Shannon Nelson 2024-02-14 609 netdev_dbg(netdev, "xdp_do_redirect err %d\n", err);
587fc3f0dceb08 Shannon Nelson 2024-02-14 610 goto out_xdp_abort;
587fc3f0dceb08 Shannon Nelson 2024-02-14 611 }
587fc3f0dceb08 Shannon Nelson 2024-02-14 612 buf_info->page = NULL;
17cb07b96a5f58 Taehee Yoo 2024-06-18 613 if (xdp_frame_has_frags(xdpf)) {
17cb07b96a5f58 Taehee Yoo 2024-06-18 614 for (i = 0; i < sinfo->nr_frags; i++) {
17cb07b96a5f58 Taehee Yoo 2024-06-18 615 buf_info++;
17cb07b96a5f58 Taehee Yoo 2024-06-18 616 dma_unmap_page(rxq->dev, buf_info->dma_addr,
17cb07b96a5f58 Taehee Yoo 2024-06-18 617 IONIC_PAGE_SIZE, DMA_FROM_DEVICE);
17cb07b96a5f58 Taehee Yoo 2024-06-18 618 buf_info->page = NULL;
17cb07b96a5f58 Taehee Yoo 2024-06-18 619 }
17cb07b96a5f58 Taehee Yoo 2024-06-18 620 }
17cb07b96a5f58 Taehee Yoo 2024-06-18 621
587fc3f0dceb08 Shannon Nelson 2024-02-14 622 rxq->xdp_flush = true;
587fc3f0dceb08 Shannon Nelson 2024-02-14 623 stats->xdp_redirect++;
587fc3f0dceb08 Shannon Nelson 2024-02-14 624 break;
587fc3f0dceb08 Shannon Nelson 2024-02-14 625
180e35cdf035d1 Shannon Nelson 2024-02-14 626 case XDP_ABORTED:
180e35cdf035d1 Shannon Nelson 2024-02-14 627 default:
8eeed8373e1cca Shannon Nelson 2024-02-14 628 goto out_xdp_abort;
8eeed8373e1cca Shannon Nelson 2024-02-14 629 }
8eeed8373e1cca Shannon Nelson 2024-02-14 630
8eeed8373e1cca Shannon Nelson 2024-02-14 631 return true;
8eeed8373e1cca Shannon Nelson 2024-02-14 632
8eeed8373e1cca Shannon Nelson 2024-02-14 633 out_xdp_abort:
180e35cdf035d1 Shannon Nelson 2024-02-14 634 trace_xdp_exception(netdev, xdp_prog, xdp_action);
180e35cdf035d1 Shannon Nelson 2024-02-14 635 ionic_rx_page_free(rxq, buf_info);
180e35cdf035d1 Shannon Nelson 2024-02-14 636 stats->xdp_aborted++;
180e35cdf035d1 Shannon Nelson 2024-02-14 637
180e35cdf035d1 Shannon Nelson 2024-02-14 638 return true;
180e35cdf035d1 Shannon Nelson 2024-02-14 639 }
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c index 2427610f4306..792e530e3281 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c @@ -487,14 +487,13 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, struct ionic_buf_info *buf_info, int len) { + int remain_len, frag_len, i, err = 0; + struct skb_shared_info *sinfo; u32 xdp_action = XDP_ABORTED; struct xdp_buff xdp_buf; struct ionic_queue *txq; struct netdev_queue *nq; struct xdp_frame *xdpf; - int remain_len; - int frag_len; - int err = 0; xdp_init_buff(&xdp_buf, IONIC_PAGE_SIZE, rxq->xdp_rxq_info); frag_len = min_t(u16, len, IONIC_XDP_MAX_LINEAR_MTU + VLAN_ETH_HLEN); @@ -513,7 +512,6 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, */ remain_len = len - frag_len; if (remain_len) { - struct skb_shared_info *sinfo; struct ionic_buf_info *bi; skb_frag_t *frag; @@ -576,7 +574,6 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, dma_unmap_page(rxq->dev, buf_info->dma_addr, IONIC_PAGE_SIZE, DMA_FROM_DEVICE); - err = ionic_xdp_post_frame(txq, xdpf, XDP_TX, buf_info->page, buf_info->page_offset, @@ -587,12 +584,22 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, goto out_xdp_abort; } buf_info->page = NULL; + if (xdp_frame_has_frags(xdpf)) { + for (i = 0; i < sinfo->nr_frags; i++) { + buf_info++; + dma_unmap_page(rxq->dev, buf_info->dma_addr, + IONIC_PAGE_SIZE, DMA_FROM_DEVICE); + buf_info->page = NULL; + } + } + stats->xdp_tx++; /* the Tx completion will free the buffers */ break; case XDP_REDIRECT: + xdpf = xdp_convert_buff_to_frame(&xdp_buf); /* unmap the pages before handing them to a different device */ dma_unmap_page(rxq->dev, buf_info->dma_addr, IONIC_PAGE_SIZE, DMA_FROM_DEVICE); @@ -603,6 +610,15 @@ static bool ionic_run_xdp(struct ionic_rx_stats *stats, goto out_xdp_abort; } buf_info->page = NULL; + if (xdp_frame_has_frags(xdpf)) { + for (i = 0; i < sinfo->nr_frags; i++) { + buf_info++; + dma_unmap_page(rxq->dev, buf_info->dma_addr, + IONIC_PAGE_SIZE, DMA_FROM_DEVICE); + buf_info->page = NULL; + } + } + rxq->xdp_flush = true; stats->xdp_redirect++; break;
Currently, the ionic_run_xdp() doesn't handle multi-buffer packets properly for XDP_TX and XDP_REDIRECT. When a jumbo frame is received, the ionic_run_xdp() first makes xdp frame with all necessary pages in the rx descriptor. And if the action is either XDP_TX or XDP_REDIRECT, it should unmap dma-mapping and reset page pointer to NULL for all pages, not only the first page. But it doesn't for SG pages. So, SG pages unexpectedly will be reused. It eventually causes kernel panic. Oops: general protection fault, probably for non-canonical address 0x504f4e4dbebc64ff: 0000 [#1] PREEMPT SMP NOPTI CPU: 3 PID: 0 Comm: swapper/3 Not tainted 6.10.0-rc3+ #25 RIP: 0010:xdp_return_frame+0x42/0x90 Code: 01 75 12 5b 4c 89 e6 5d 31 c9 41 5c 31 d2 41 5d e9 73 fd ff ff 44 8b 6b 20 0f b7 43 0a 49 81 ed 68 01 00 00 49 29 c5 49 01 fd <41> 80 7d0 RSP: 0018:ffff99d00122ce08 EFLAGS: 00010202 RAX: 0000000000005453 RBX: ffff8d325f904000 RCX: 0000000000000001 RDX: 00000000670e1000 RSI: 000000011f90d000 RDI: 504f4e4d4c4b4a49 RBP: ffff99d003907740 R08: 0000000000000000 R09: 0000000000000000 R10: 000000011f90d000 R11: 0000000000000000 R12: ffff8d325f904010 R13: 504f4e4dbebc64fd R14: ffff8d3242b070c8 R15: ffff99d0039077c0 FS: 0000000000000000(0000) GS:ffff8d399f780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f41f6c85e38 CR3: 000000037ac30000 CR4: 00000000007506f0 PKRU: 55555554 Call Trace: <IRQ> ? die_addr+0x33/0x90 ? exc_general_protection+0x251/0x2f0 ? asm_exc_general_protection+0x22/0x30 ? xdp_return_frame+0x42/0x90 ionic_tx_clean+0x211/0x280 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] ionic_tx_cq_service+0xd3/0x210 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] ionic_txrx_napi+0x41/0x1b0 [ionic 15881354510e6a9c655c59c54812b319ed2cd015] __napi_poll.constprop.0+0x29/0x1b0 net_rx_action+0x2c4/0x350 handle_softirqs+0xf4/0x320 irq_exit_rcu+0x78/0xa0 common_interrupt+0x77/0x90 Fixes: 5377805dc1c0 ("ionic: implement xdp frags support") Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- In the XDP_DROP and XDP_ABORTED path, the ionic_rx_page_free() is called to free page and unmap dma-mapping. It resets only the first page pointer to NULL. DROP/ABORTED path looks like it also has the same problem, but it's not. Because all pages in the XDP_DROP/ABORTED path are not used anywhere it can be reused without any free and unmap. So, it's okay to remove the function in the xdp path. I will send a separated patch for removing ionic_rx_page_free() in the xdp path. .../net/ethernet/pensando/ionic/ionic_txrx.c | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-)