netdev - Re: [RFC PATCH v5] vmxnet3: Add XDP support.

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Dec 2022 14:51:27 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     William Tu <u9012063@...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, 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.

> > 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.

> +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.