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 13:55:43 -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.

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.

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

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?

Thanks
William