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]

Message-ID: <c8f874c6-3271-6bd1-f3b9-4d0b0786cd52@digitalocean.com>
Date:   Wed, 26 Feb 2020 09:08:57 -0700
From:   David Ahern <dahern@...italocean.com>
To:     "Michael S. Tsirkin" <mst@...hat.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 2/26/20 12:07 AM, Michael S. Tsirkin wrote:
> 
> 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.

Not necessarily.

> 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.

I grabbed that from virtnet_probe() for sake of this discussion:

        /* MTU range: 68 - 65535 */
        dev->min_mtu = MIN_MTU;
        dev->max_mtu = MAX_MTU;

but yes I see the MTU probe now, so I guess that could be used instead
of MAX_MTU.

> 
> 
>> 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?
> 

consistency. If other change_mtu functions fail EINVAL, then virtio net
needs to follow suit.