netdev - Re: [PATCH RFC net-next v19] 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] [day] [month] [year] [list]

Message-ID: <CALDO+Sac1_QnZgBo6SoyCrEY5-VG-rGXuutVY5GJrgxXRSsHkA@mail.gmail.com>
Date:   Wed, 12 Apr 2023 16:29:32 -0700
From:   William Tu <u9012063@...il.com>
To:     Alexander Lobakin <aleksander.lobakin@...el.com>
Cc:     netdev@...r.kernel.org, jsankararama@...are.com, gyang@...are.com,
        doshir@...are.com, alexander.duyck@...il.com,
        alexandr.lobakin@...el.com, bang@...are.com,
        maciej.fijalkowski@...el.com, witu@...dia.com,
        horatiu.vultur@...rochip.com, error27@...il.com,
        Alexander Duyck <alexanderduyck@...com>
Subject: Re: [PATCH RFC net-next v19] vmxnet3: Add XDP support.

Hi Alexander,

Sorry for my late reply and thanks for taking another round of review!

On Fri, Mar 31, 2023 at 8:43 AM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
>
> From: William Tu <u9012063@...il.com>
> Date: Sat, 25 Mar 2023 10:28:28 -0700
>
> Sorry for the late reply, I've been busy :s
>
> > From: William Tu <u9012063@...il.com>
> >
> > The patch adds native-mode XDP support: XDP DROP, PASS, TX, and REDIRECT.
> >
> > Background:
>
> [...]
>
> > +static int
> > +vmxnet3_run_xdp(struct vmxnet3_rx_queue *rq, struct xdp_buff *xdp,
> > +             struct bpf_prog *prog)
> > +{
> > +     struct xdp_frame *xdpf;
> > +     struct page *page;
> > +     int err;
> > +     u32 act;
> > +
> > +     act = bpf_prog_run_xdp(prog, xdp);
> > +     rq->stats.xdp_packets++;
>
> I think you can increment it *before* running the program, so that
> there'll be as tiny time gap as possible.

Good idea, will do it.

>
> > +     page = virt_to_page(xdp->data_hard_start);
>
> You don't need it for PASS and REDIRECT.
>
> > +
> > +     switch (act) {
> > +     case XDP_PASS:
> > +             return act;
> > +     case XDP_REDIRECT:
> > +             err = xdp_do_redirect(rq->adapter->netdev, xdp, prog);
> > +             if (!err)
> > +                     rq->stats.xdp_redirects++;
> > +             else
> > +                     rq->stats.xdp_drops++;
>
> BTW, if you get @err here, shouldn't you recycle the page, just like in
> TX case?

Yes, will fix it.

>
> > +             return act;
> > +     case XDP_TX:
> > +             xdpf = xdp_convert_buff_to_frame(xdp);
> > +             if (unlikely(!xdpf ||
> > +                          vmxnet3_xdp_xmit_back(rq->adapter, xdpf))) {
> > +                     rq->stats.xdp_drops++;
> > +                     page_pool_recycle_direct(rq->page_pool, page);
> > +             } else {
> > +                     rq->stats.xdp_tx++;
> > +             }
> > +             return act;
> > +     default:
> > +             bpf_warn_invalid_xdp_action(rq->adapter->netdev, prog, act);
> > +             fallthrough;
> > +     case XDP_ABORTED:
> > +             trace_xdp_exception(rq->adapter->netdev, prog, act);
> > +             rq->stats.xdp_aborted++;
> > +             break;
> > +     case XDP_DROP:
> > +             rq->stats.xdp_drops++;
> > +             break;
> > +     }
> > +
> > +     page_pool_recycle_direct(rq->page_pool, page);
> > +
> > +     return act;
> > +}
>
> [...]
>
> > +     xdp_init_buff(&xdp, PAGE_SIZE, &rq->xdp_rxq);
> > +     xdp_prepare_buff(&xdp, page_address(page), rq->page_pool->p.offset,
> > +                      len, false);
> > +     xdp_buff_clear_frags_flag(&xdp);
> > +
> > +     /* Must copy the data because it's at dataring. */
> > +     memcpy(xdp.data, data, len);
> > +
> > +     rcu_read_lock();
>
> Where's the corresponding unlock?
I should remove this rcu_read_lock. A mistake in v16
    - remove using rcu_read_lock,unlock around XDP invocation
      https://lore.kernel.org/bpf/20210624160609.292325-1-toke@redhat.com/
>
> > +     xdp_prog = rcu_dereference(rq->adapter->xdp_bpf_prog);
> > +     if (!xdp_prog) {
> > +             act = XDP_PASS;
> > +             goto out_skb;
> > +     }
> > +     act = vmxnet3_run_xdp(rq, &xdp, xdp_prog);
> > +
> > +     if (act == XDP_PASS) {
> > +out_skb:
> > +             *skb_xdp_pass = vmxnet3_build_skb(rq, page, &xdp);
> > +             if (!*skb_xdp_pass)
> > +                     return XDP_DROP;
> > +     }
> > +
> > +     /* No need to refill. */
> > +     return act;
>
> Maybe
>
>         act = vmxnet3_run_xdp(rq, &xdp, xdp_prog);
>         if (act != XDP_PASS)
>                 return act;
>
> out_skb:
>         *skb_xdp_pass = vmxnet3_build_skb(rq, page, &xdp);
>
>         return likely(*skb_xdp_pass) ? act : XDP_DROP;

it does look simpler, will use this.

thanks
William