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] [day] [month] [year] [list]

Date:   Thu, 15 Dec 2022 11:59:53 -0800
From:   Alexander 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 Thu, Dec 15, 2022 at 10:25 AM William Tu <u9012063@...il.com> wrote:
>
> On Wed, Dec 14, 2022 at 2:51 PM Alexander H Duyck
> <alexander.duyck@...il.com> wrote:
> >
> > On Wed, 2022-12-14 at 13:55 -0800, William Tu wrote:
> > > Thanks for taking a look at this patch!
> > >
> > > <...>

> > > 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.
> I see your point. So for XDP_TX, I can also do s.t like I did in XDP_REDIRECT,
> memcpy to a freshly allocated page so the frame won't get overwritten.
> Probably the performance will suffer.
> Do you suggest allocating new page or risk buffer overwritten?

This is one of the reasons for the page pool being used in other
drivers. You may want to look at going that route if you want to avoid
the memcpy. I don't think you can leave the page mapped without
risking it will be overwritten.

> >
> > > +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.
>
> You're right! I don't need to pass skb here. I probably forgot to remove it
> when refactoring code. Will remove the two places.
> Thanks!

Sounds good. I will keep an eye out for v6.