netdev - Re: [PATCH v6] 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 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]
Date:   Thu, 22 Dec 2022 07:58:02 -0800
From:   William Tu <u9012063@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     netdev@...r.kernel.org, tuc@...are.com, gyang@...are.com,
        doshir@...are.com
Subject: Re: [PATCH v6] vmxnet3: Add XDP support.

> > > > > +     return nxmit;
> > > > > +}
> > > > > +
> > > > > +static int
> > > > > +__vmxnet3_run_xdp(struct vmxnet3_rx_queue *rq, void *data, int data_len,
> > > > > +               int headroom, int frame_sz, bool *need_xdp_flush,
> > > > > +               struct sk_buff *skb)
> > > > > +{
> > > > > +     struct xdp_frame *xdpf;
> > > > > +     void *buf_hard_start;
> > > > > +     struct xdp_buff xdp;
> > > > > +     struct page *page;
> > > > > +     void *orig_data;
> > > > > +     int err, delta;
> > > > > +     int delta_len;
> > > > > +     u32 act;
> > > > > +
> > > > > +     buf_hard_start = data;
> > > > > +     xdp_init_buff(&xdp, frame_sz, &rq->xdp_rxq);
> > > > > +     xdp_prepare_buff(&xdp, buf_hard_start, headroom, data_len, true);
> > > > > +     orig_data = xdp.data;
> > > > > +
> > > > > +     act = bpf_prog_run_xdp(rq->xdp_bpf_prog, &xdp);
> > > > > +     rq->stats.xdp_packets++;
> > > > > +
> > > > > +     switch (act) {
> > > > > +     case XDP_DROP:
> > > > > +             rq->stats.xdp_drops++;
> > > > > +             break;
> > > > > +     case XDP_PASS:
> > > > > +             /* bpf prog might change len and data position.
> > > > > +              * dataring does not use skb so not support this.
> > > > > +              */
> > > > > +             delta = xdp.data - orig_data;
> > > > > +             delta_len = (xdp.data_end - xdp.data) - data_len;
> > > > > +             if (skb) {
> > > > > +                     skb_reserve(skb, delta);
> > > > > +                     skb_put(skb, delta_len);
> > > > > +             }
> > > > > +             break;
> > > > > +     case XDP_TX:
> > > > > +             xdpf = xdp_convert_buff_to_frame(&xdp);
> > > > > +             if (!xdpf ||
> > > > > +                 vmxnet3_xdp_xmit_back(rq->adapter, xdpf)) {
> > > > > +                     rq->stats.xdp_drops++;
> > > > > +             } else {
> > > > > +                     rq->stats.xdp_tx++;
> > > > > +             }
> > > > > +             break;
> > > > > +     case XDP_ABORTED:
> > > > > +             trace_xdp_exception(rq->adapter->netdev, rq->xdp_bpf_prog,
> > > > > +                                 act);
> > > > > +             rq->stats.xdp_aborted++;
> > > > > +             break;
> > > > > +     case XDP_REDIRECT:
> > > > > +             page = alloc_page(GFP_ATOMIC);
> > > > > +             if (!page) {
> > > > > +                     rq->stats.rx_buf_alloc_failure++;
> > > > > +                     return XDP_DROP;
> > > > > +             }
> > > >
> > > > So I think I see the problem I had questions about here. If I am not
> > > > mistaken you are copying the buffer to this page, and then copying this
> > > > page to an skb right? I think you might be better off just writing off
> > > > the Tx/Redirect pages and letting them go through their respective
> > > > paths and just allocating new pages instead assuming these pages were
> > > > consumed.

Hi Alexander,

I tried to implement your suggestion above: remove the alloc_page, let
the original
pages go through the redirect path (the xdp_do_redirect), and then allocate new
buffer to refill rx ring but hit some memleak issue.
The XDP_PASS/DROP/TX are good without leak, but not REDIRECT.
I sent out v7, do you mind taking another look?

Thanks!
William


> > >
> > > I'm not sure I understand, can you elaborate?
> > >
> > > For XDP_TX, I'm doing 1 extra copy, copying to the newly allocated skb
> > > in vmxnet3_xdp_xmit_back.
> > > For XDP_REDIREC, I allocate a page and copy to the page and call
> > > xdp_do_redirect, there is no copying to skb again. If I don't allocate a
> > > new page, it always crashes at
> > > [62020.425932] BUG: Bad page state in process cpumap/0/map:29  pfn:107548
> > > [62020.440905] kernel BUG at include/linux/mm.h:757!
> > >  VM_BUG_ON_PAGE(page_ref_count(page) == 0, page);
> >
> > What I was referring to was one copy here, and then another copy in
> > your vmxnet3_xdp_xmit_frame() function with the page you allocated
> > here possibly being leaked.
> >
> > Though based on the other trace you provided it would look like you
> > are redirecting to something else as your code currently doesn't
> > reference xdp_return_frame which is what I was referring to earlier in
> > terms of missing the logic to free the frame in your transmit path.
> yes, I tried a couple ways and now it's working.
> basically I have to call get_page to increment the refcnt then call
> xdp_return_frame. I will update performance number and send
> next version.
>
> Thanks
> William