netdev - Re: [RFC PATCH v7] 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]

Message-ID: <CALDO+SZay_vErqqikAvA=p8nEGUT+1gkR3EyKMdObQqPyKGERg@mail.gmail.com>
Date:   Sun, 25 Dec 2022 11:09:37 -0800
From:   William Tu <u9012063@...il.com>
To:     Gerhard Engleder <gerhard@...leder-embedded.com>
Cc:     netdev@...r.kernel.org, tuc@...are.com, gyang@...are.com,
        doshir@...are.com
Subject: Re: [RFC PATCH v7] vmxnet3: Add XDP support.

Hi Gerhard,

Thanks for taking a look at this patch!


On Thu, Dec 22, 2022 at 1:25 PM Gerhard Engleder
<gerhard@...leder-embedded.com> wrote:
>
> On 22.12.22 16:46, William Tu wrote:
> > @@ -1776,6 +1800,7 @@ vmxnet3_rq_cleanup(struct vmxnet3_rx_queue *rq,
> >
> >       rq->comp_ring.gen = VMXNET3_INIT_GEN;
> >       rq->comp_ring.next2proc = 0;
> > +     rq->xdp_bpf_prog = NULL;
>
> Reference to BPF program is lost without calling bpf_prog_put(). Are you
> sure this is ok? This function is called during ndo_stop too.

sure, I will do it!

>
> [...]
>
> > +static int
> > +vmxnet3_xdp_set(struct net_device *netdev, struct netdev_bpf *bpf,
> > +             struct netlink_ext_ack *extack)
> > +{
> > +     struct vmxnet3_adapter *adapter = netdev_priv(netdev);
> > +     struct bpf_prog *new_bpf_prog = bpf->prog;
> > +     struct bpf_prog *old_bpf_prog;
> > +     bool need_update;
> > +     bool running;
> > +     int err = 0;
> > +
> > +     if (new_bpf_prog && netdev->mtu > VMXNET3_XDP_MAX_MTU) {
> > +             NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     old_bpf_prog = READ_ONCE(adapter->rx_queue[0].xdp_bpf_prog);
>
> Wouldn't it be simpler if xdp_bpf_prog is move from rx_queue to
> adapter?
Yes, I think it's easier to keep xdp_bpf_prog in adapter instead of
one in each rq, since it's all the same bpf prog.

>
> [...]
>
> > +/* This is the main xdp call used by kernel to set/unset eBPF program. */
> > +int
> > +vmxnet3_xdp(struct net_device *netdev, struct netdev_bpf *bpf)
> > +{
> > +     switch (bpf->command) {
> > +     case XDP_SETUP_PROG:
> > +             netdev_dbg(netdev, "XDP: set program to ");
>
> Did you forget to delete this debug output?
Yes, thanks!

>
> [...]
>
> > +int
> > +vmxnet3_xdp_xmit(struct net_device *dev,
> > +              int n, struct xdp_frame **frames, u32 flags)
> > +{
> > +     struct vmxnet3_adapter *adapter;
> > +     struct vmxnet3_tx_queue *tq;
> > +     struct netdev_queue *nq;
> > +     int i, err, cpu;
> > +     int nxmit = 0;
> > +     int tq_number;
> > +
> > +     adapter = netdev_priv(dev);
> > +
> > +     if (unlikely(test_bit(VMXNET3_STATE_BIT_QUIESCED, &adapter->state)))
> > +             return -ENETDOWN;
> > +     if (unlikely(test_bit(VMXNET3_STATE_BIT_RESETTING, &adapter->state)))
> > +             return -EINVAL;
> > +
> > +     tq_number = adapter->num_tx_queues;
> > +     cpu = smp_processor_id();
> > +     tq = &adapter->tx_queue[cpu % tq_number];
> > +     if (tq->stopped)
> > +             return -ENETDOWN;
> > +
> > +     nq = netdev_get_tx_queue(adapter->netdev, tq->qid);
> > +
> > +     __netif_tx_lock(nq, cpu);
> > +     for (i = 0; i < n; i++) {
> > +             err = vmxnet3_xdp_xmit_frame(adapter, frames[i], tq);
> > +             /* vmxnet3_xdp_xmit_frame has copied the data
> > +              * to skb, so we free xdp frame below.
> > +              */
> > +             get_page(virt_to_page(frames[i]->data));
> > +             xdp_return_frame(frames[i]);
> > +             if (err) {
> > +                     tq->stats.xdp_xmit_err++;
> > +                     break;
> > +             }
> > +             nxmit++;
>
> You could just use the loop iterator and drop nxmit. I got this comment
> for one of my XDP patches from Saeed Mahameed.
Good idea.

>
> [...]
>
> Did you consider to split this patch into multiple patches to make
> review easier?

Sure, but I still want to make each patch as a fully working feature.
I will probably separate the XDP_REDIRECT feature into another one.


William