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

Message-ID: <CAGnkfhzvJqYzxd4_ZSt6NzFXx+8Uxrjr=HUGg7=-UQFzMDjN3w@mail.gmail.com>
Date:   Fri, 5 Apr 2019 17:45:34 +0200
From:   Matteo Croce <mcroce@...hat.com>
To:     David Miller <davem@...emloft.net>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     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>
Subject: Re: [PATCH net] net: thunderx: don't allow jumbo frames with XDP

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.


Regards,
--
Matteo Croce
per aspera ad upstream