Re: virtio_net: can change MTU after installing program

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Feb 26, 2020 at 09:08:57AM -0700, David Ahern wrote:
> 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.

Maybe we should change them all to EBUSY - that's not too hard ...

-- 
MST