netdev - Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP

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:   Sat, 6 Apr 2019 18:19:29 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Matteo Croce <mcroce@...hat.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Sunil Goutham <sgoutham@...ium.com>,
        Robert Richter <rric@...nel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        brouer@...hat.com
Subject: Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP

On Fri, 5 Apr 2019 17:45:34 +0200
Matteo Croce <mcroce@...hat.com> wrote:

> On Fri, Apr 5, 2019 at 2:51 AM Matteo Croce <mcroce@...hat.com> wrote:
> >
> > On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@...emloft.net> wrote:  
> > >
> > > From: Matteo Croce <mcroce@...hat.com>
> > > Date: Wed,  3 Apr 2019 01:11:36 +0200
> > >  
> > > > The thunderx driver forbids to load an eBPF program if the MTU is
> > > > higher than 1500 bytes, but this can be circumvented by first
> > > > loading the eBPF, and then raising the MTU.
> > > >
> > > > XDP assumes that SKBs are linear and fit in a single page, this can
> > > > lead to undefined behaviours.
> > > > Fix this by limiting the MTU to 1500 bytes if an eBPF program is
> > > > loaded.
> > > >
> > > > Fixes: 05c773f52b96e ("net: thunderx: Add basic XDP support")
> > > > Signed-off-by: Matteo Croce <mcroce@...hat.com>  
> > >
> > > Please respond to Jesper's feedback about your choice of a limit of
> > > 1500.
> > >
> > > Otherwise I will toss your patch.  
> >
> > Hi David ad Jesper,
> >
> > I didn't deliberately choose a limit of 1500, the limit is always set
> > in nicvf_xdp_setup():
> >
> >     /* For now just support only the usual MTU sized frames */
> >     if (prog && (dev->mtu > 1500)) {
> >         netdev_warn(dev, "Jumbo frames not yet supported with XDP...
> >
> > I just enforced the same limit in another code path which didn't do
> > the check.
> > If you think that 1500 is a bad value, and I'm sure you're right because
> > there isn't room even for VLAN tagging, I will send a series like:
> > - 1/2 sets the limit to a resonable value
> > - 2/2 enforce the same limit in the two code paths
> >
> > Regards,
> > --
> > Matteo Croce
> > per aspera ad upstream  
> 
> Hi all,
> 
> I did some tests and I've found that on this driver, the maximum
> allowed frame size with XDP is 1530.
> Frames bigger than 1530 are split around multiple pages, so the driver
> doesn't even run the bpf on them:
> 
>         /* For XDP, ignore pkts spanning multiple pages */
>         if (nic->xdp_prog && (cqe_rx->rb_cnt == 1)) {
> 
> based on this test, I'll send a series with a proper MTU limit which
> should be, correct me if I'm wrong: 1530 - 14 (eth) - 4 (QinQ) = 1512
> bytes.
> I subtract only the 4 bytes for the QinQ as the
> NETIF_F_VLAN_CHALLENGED_BIT flag is not set, and the first VLAN tag
> should not be counted.

You *do* need to include the first VLAN tag in the calculation.
I guess I didn't explain this clear enough on IRC.

XDP cannot use VLAN-offloading. As we explain here[1] when running XDP,
you need to disable VLAN-offloading (see cmd in [1]), because XDP need
the VLAN header to be "inline" in the packet.  XDP don't (yet) have
access to reading info from the descriptor.

[1] https://github.com/xdp-project/xdp-tutorial/tree/master/packet01-parsing#a-note-about-vlan-offloads
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer