lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Dec 2022 10:24:49 -0800 From: William Tu <u9012063@...il.com> To: Alexander H Duyck <alexander.duyck@...il.com> Cc: netdev@...r.kernel.org, tuc@...are.com, gyang@...are.com, doshir@...are.com Subject: Re: [RFC PATCH v5] vmxnet3: Add XDP support. On Wed, Dec 14, 2022 at 2:51 PM Alexander H Duyck <alexander.duyck@...il.com> wrote: > > On Wed, 2022-12-14 at 13:55 -0800, William Tu wrote: > > Thanks for taking a look at this patch! > > > > <...> > > > > > > > +int > > > > +vmxnet3_process_xdp(struct vmxnet3_adapter *adapter, > > > > + struct vmxnet3_rx_queue *rq, > > > > + struct Vmxnet3_RxCompDesc *rcd, > > > > + struct vmxnet3_rx_buf_info *rbi, > > > > + struct Vmxnet3_RxDesc *rxd, > > > > + bool *need_flush) > > > > +{ > > > > + struct bpf_prog *xdp_prog; > > > > + dma_addr_t new_dma_addr; > > > > + struct sk_buff *new_skb; > > > > + bool rxDataRingUsed; > > > > + int ret, act; > > > > + > > > > + ret = VMXNET3_XDP_CONTINUE; > > > > + if (unlikely(rcd->len == 0)) > > > > + return VMXNET3_XDP_TAKEN; > > > > + > > > > + rxDataRingUsed = VMXNET3_RX_DATA_RING(adapter, rcd->rqID); > > > > + rcu_read_lock(); > > > > + xdp_prog = rcu_dereference(rq->xdp_bpf_prog); > > > > + if (!xdp_prog) { > > > > + rcu_read_unlock(); > > > > + return VMXNET3_XDP_CONTINUE; > > > > + } > > > > + act = vmxnet3_run_xdp(rq, rbi, rcd, need_flush, rxDataRingUsed); > > > > + rcu_read_unlock(); > > > > + > > > > + switch (act) { > > > > + case XDP_PASS: > > > > + ret = VMXNET3_XDP_CONTINUE; > > > > + break; > > > > + case XDP_DROP: > > > > + case XDP_TX: > > > > + case XDP_REDIRECT: > > > > + case XDP_ABORTED: > > > > + default: > > > > + /* Reuse and remap the existing buffer. */ > > > > + ret = VMXNET3_XDP_TAKEN; > > > > + if (rxDataRingUsed) > > > > + return ret; > > > > + > > > > + new_skb = rbi->skb; > > > > + new_dma_addr = > > > > + dma_map_single(&adapter->pdev->dev, > > > > + new_skb->data, rbi->len, > > > > + DMA_FROM_DEVICE); > > > > + if (dma_mapping_error(&adapter->pdev->dev, > > > > + new_dma_addr)) { > > > > + dev_kfree_skb(new_skb); > > > > + rq->stats.drop_total++; > > > > + return ret; > > > > + } > > > > + rbi->dma_addr = new_dma_addr; > > > > + rxd->addr = cpu_to_le64(rbi->dma_addr); > > > > + rxd->len = rbi->len; > > > > + } > > > > + return ret; > > > > +} > > > > > > FOr XDP_DROP and XDP_ABORTED this makes sense. You might want to add a > > > trace point in the case of aborted just so you can catch such cases for > > > debug. > > Good point, I will add the trace point. > > > > You will probably want to add that trace point in __vmxnet3_run_xdp. Yes, thanks. > > > > However for XDP_TX and XDP_REDIRECT shouldn't both of those be calling > > > out to seperate functions to either place the frame on a Tx ring or to > > > hand it off to xdp_do_redirect so that the frame gets routed where it > > > needs to go? Also don't you run a risk with overwriting frames that > > > might be waiting on transmit? > > > > Yes, I have XDP_TX and XDP_REDIRECT handled in another function, > > the vmxnet3_run_xdp() and __vmxnet3_run_xdp(). > > Okay, for the redirect case it looks like you address it by doing a > memcpy to a freshly allocated page so that saves us that trouble in > that case. > > > How do I avoid overwriting frames that might be waiting on transmit? > > I checked my vmxnet3_xdp_xmit_back and vmxnet3_xdp_xmit_frame, > > I think since I called the vmxnet3_xdp_xmit_frame at the rx context, > > it should be ok? > > I don't think you can guarantee that. Normally for TX you would want to > detach and replace the page unless you have some sort of other > recycling/reuse taking care of it for you. Normally that is handled via > page pool. > > On the Intel parts I had gotten around that via our split buffer model > so we just switched to the other half of the page while the Tx sat on > the first half, and by the time we would have to check again we would > either detach the page for flip back if it had already been freed by > the Tx path. I see your point. So for XDP_TX, I can also do s.t like I did in XDP_REDIRECT, memcpy to a freshly allocated page so the frame won't get overwritten. Probably the performance will suffer. Do you suggest allocating new page or risk buffer overwritten? > > > +static int > > +vmxnet3_xdp_xmit_back(struct vmxnet3_adapter *adapter, > > + struct xdp_frame *xdpf, > > + struct sk_buff *skb) > > > > Also after re-reviewing this I was wondering why you have the skb > argument for this function? The only caller is passing NULL and I > wouldn't expect you to be passing an skb since you are working with XDP > buffers anyway. Seems like you could also drop the argument from > vmxnet3_xdp_xmit_frame() since you are only passing it NULL as well. You're right! I don't need to pass skb here. I probably forgot to remove it when refactoring code. Will remove the two places. Thanks! Regards, William
Powered by blists - more mailing lists