netdev - Re: virtio_net: can change MTU after installing program

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]

Date:   Wed, 26 Feb 2020 02:07:04 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     David Ahern <dahern@...italocean.com>
Cc:     Jason Wang <jasowang@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: virtio_net: can change MTU after installing program

On Tue, Feb 25, 2020 at 08:32:14PM -0700, David Ahern wrote:
> Another issue is that virtio_net checks the MTU when a program is
> installed, but does not restrict an MTU change after:
> 
> # ip li sh dev eth0
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 xdp qdisc fq_codel
> state UP mode DEFAULT group default qlen 1000
>     link/ether 5a:39:e6:01:a5:36 brd ff:ff:ff:ff:ff:ff
>     prog/xdp id 13 tag c5595e4590d58063 jited
> 
> # ip li set dev eth0 mtu 8192
> 
> # ip li sh dev eth0
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 8192 xdp qdisc fq_codel
> state UP mode DEFAULT group default qlen 1000

Well the reason XDP wants to limit MTU is this:
    the MTU must be less than a page
    size to avoid having to handle XDP across multiple pages

however device mtu basically comes from dhcp.
it is assumed that whoever configured it knew
what he's doing and configured mtu to match
what's going on on the underlying backend.
So we are trusting the user already.

But yes, one can configure mtu later and then it's too late
as xdp was attached.


> 
> 
> The simple solution is:
> 
> @@ -2489,6 +2495,8 @@ static int virtnet_xdp_set(struct net_device *dev,
> struct bpf_prog *prog,
>                 }
>         }
> 
> +       dev->max_mtu = prog ? max_sz : MAX_MTU;
> +
>         return 0;
> 
>  err:


Well max MTU comes from the device ATM and supplies the limit
of the underlying backend. Why is it OK to set it to MAX_MTU?
That's just asking for trouble IMHO, traffic will not
be packetized properly.


> The complicated solution is to implement ndo_change_mtu.
> 
> The simple solution causes a user visible change with 'ip -d li sh' by
> showing a changing max mtu, but the ndo has a poor user experience in
> that it just fails EINVAL (their is no extack) which is confusing since,
> for example, 8192 is a totally legit MTU. Changing the max does return a
> nice extack message.

Just fail with EBUSY instead?

-- 
MST