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

[Jesper Dangaard Brouer <brouer@redhat.com>]

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

> On Fri, Apr 5, 2019 at 2:51 AM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > On Fri, Apr 5, 2019 at 2:20 AM David Miller <davem@davemloft.net> wrote:  
> > >
> > > From: Matteo Croce <mcroce@redhat.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@redhat.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