(no subject)

[Michael S. Tsirkin]

Bcc: 
Subject: Re: virtio_net: can change MTU after installing program
Message-ID: <20200226043257-mutt-send-email-mst@kernel.org>
Reply-To: 
In-Reply-To: <7df5bb7f-ea69-7673-642b-f174e45a1e64@digitalocean.com>

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

Cc Toke who has tested this on other cards and has some input.

-- 
MST

virtio_net: can change MTU after installing program

[David Ahern]

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


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:

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.

Re: virtio_net: can change MTU after installing program

[Jason Wang]

On 2020/2/26 上午11:32, 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
>
>
> 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:
>
> The complicated solution is to implement ndo_change_mtu.


Right, this issue has been there for years. Qemu (or libvirt) should 
configure TAP MTU accordingly. But there's no way for driver to tell 
device about the MTU it wants to use. We need fix this.


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


Or for simplicity, just forbid changing MTU when program is installed?

Thanks


>

Re: virtio_net: can change MTU after installing program

[David Ahern]

On 2/25/20 9:02 PM, Jason Wang wrote:
>>
>> 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.
> 
> 
> Or for simplicity, just forbid changing MTU when program is installed?

My preference is to show the reduced max MTU when a program is
installed. Allows the mtu to be increased to the max XDP allows for the
device.

Re: virtio_net: can change MTU after installing program

[Jason Wang]

On 2020/2/26 下午12:31, David Ahern wrote:
> On 2/25/20 9:02 PM, Jason Wang wrote:
>>> 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.
>>
>> Or for simplicity, just forbid changing MTU when program is installed?
> My preference is to show the reduced max MTU when a program is
> installed. Allows the mtu to be increased to the max XDP allows for the
> device.
>

Yes, that's better. But may requires changes in both qemu and virtio 
spec to work.

Do you want to address them? Or if it's not urgent, I can add this in my 
todo list and address it in the future.

Thanks

Re: virtio_net: can change MTU after installing program

[David Ahern]

On 2/25/20 10:53 PM, Jason Wang wrote:
> Yes, that's better. But may requires changes in both qemu and virtio
> spec to work.
> 
> Do you want to address them? Or if it's not urgent, I can add this in my
> todo list and address it in the future.

I do not understand why a virtio spec change is required, so sounds like
something you should take care of.

Re: virtio_net: can change MTU after installing program

[Michael S. Tsirkin]

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

Re: virtio_net: can change MTU after installing program

[Jason Wang]

----- Original Message -----
> 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
> 

But even if we limit MTU is guest there's no way to limit the packet
size on host. It looks to me we need to introduce new commands to
change the backend MTU (e.g TAP) accordingly.

Thanks

Re: virtio_net: can change MTU after installing program

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 02:37:01AM -0500, Jason Wang wrote:
> 
> 
> ----- Original Message -----
> > 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
> > 
> 
> But even if we limit MTU is guest there's no way to limit the packet
> size on host.

Isn't this fundamental? IIUC dev->mtu is mostly a hint to devices about
how the network is configured. It has to be the same across LAN.  If
someone misconfigures it that breaks networking, and user gets to keep
both pieces. E.g. e1000 will use dev->mtu to calculate rx buffer size.
If you make it too small, well packets that are too big get dropped.
There's no magic to somehow make them smaller, or anything like that.
We can certainly drop packet > dev->mtu in the driver right now if we want to,
and maybe if it somehow becomes important for performance, we
could teach host to drop such packets for us. Though
I don't really see why we care ...

> It looks to me we need to introduce new commands to
> change the backend MTU (e.g TAP) accordingly.
> 
> Thanks

So you are saying there are configurations where host does not know the
correct MTU, and needs guest's help to figure it out? I guess it's
possible but it seems beside the point raised here.  TAP in particular
mostly just seems to ignore MTU, I am not sure why we should bother
propagating it there from guest or host. Propagating it from guest to
the actual NIC might be useful e.g. for buffer sizing, but is tricky
to do safely in case the NIC is shared between VMs.

-- 
MST

Re: virtio_net: can change MTU after installing program

[Jason Wang]

On 2020/2/26 下午4:39, Michael S. Tsirkin wrote:
> On Wed, Feb 26, 2020 at 02:37:01AM -0500, Jason Wang wrote:
>>
>> ----- Original Message -----
>>> 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
>>>
>> But even if we limit MTU is guest there's no way to limit the packet
>> size on host.
> Isn't this fundamental? IIUC dev->mtu is mostly a hint to devices about
> how the network is configured. It has to be the same across LAN.  If
> someone misconfigures it that breaks networking, and user gets to keep
> both pieces. E.g. e1000 will use dev->mtu to calculate rx buffer size.
> If you make it too small, well packets that are too big get dropped.
> There's no magic to somehow make them smaller, or anything like that.
> We can certainly drop packet > dev->mtu in the driver right now if we want to,
> and maybe if it somehow becomes important for performance, we
> could teach host to drop such packets for us. Though
> I don't really see why we care ...
>
>> It looks to me we need to introduce new commands to
>> change the backend MTU (e.g TAP) accordingly.
>>
>> Thanks
> So you are saying there are configurations where host does not know the
> correct MTU, and needs guest's help to figure it out?


Yes.


> I guess it's
> possible but it seems beside the point raised here.  TAP in particular
> mostly just seems to ignore MTU, I am not sure why we should bother
> propagating it there from guest or host. Propagating it from guest to
> the actual NIC might be useful e.g. for buffer sizing, but is tricky
> to do safely in case the NIC is shared between VMs.


Macvlan passthrough mode could be easier I guess.

Thanks

Re: virtio_net: can change MTU after installing program

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 05:30:18PM +0800, Jason Wang wrote:
> 
> On 2020/2/26 下午4:39, Michael S. Tsirkin wrote:
> > On Wed, Feb 26, 2020 at 02:37:01AM -0500, Jason Wang wrote:
> > > 
> > > ----- Original Message -----
> > > > 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
> > > > 
> > > But even if we limit MTU is guest there's no way to limit the packet
> > > size on host.
> > Isn't this fundamental? IIUC dev->mtu is mostly a hint to devices about
> > how the network is configured. It has to be the same across LAN.  If
> > someone misconfigures it that breaks networking, and user gets to keep
> > both pieces. E.g. e1000 will use dev->mtu to calculate rx buffer size.
> > If you make it too small, well packets that are too big get dropped.
> > There's no magic to somehow make them smaller, or anything like that.
> > We can certainly drop packet > dev->mtu in the driver right now if we want to,
> > and maybe if it somehow becomes important for performance, we
> > could teach host to drop such packets for us. Though
> > I don't really see why we care ...
> > 
> > > It looks to me we need to introduce new commands to
> > > change the backend MTU (e.g TAP) accordingly.
> > > 
> > > Thanks
> > So you are saying there are configurations where host does not know the
> > correct MTU, and needs guest's help to figure it out?
> 
> 
> Yes.
> 
> 
> > I guess it's
> > possible but it seems beside the point raised here.  TAP in particular
> > mostly just seems to ignore MTU, I am not sure why we should bother
> > propagating it there from guest or host. Propagating it from guest to
> > the actual NIC might be useful e.g. for buffer sizing, but is tricky
> > to do safely in case the NIC is shared between VMs.
> 
> 
> Macvlan passthrough mode could be easier I guess.
> 
> Thanks

As usual :) So sure, it's doable for simple configs.
But this seems orthogoal to the question raised in this thread.

-- 
MST

Re: virtio_net: can change MTU after installing program

[David Ahern]

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.

Re: virtio_net: can change MTU after installing program

[Michael S. Tsirkin]

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

Re: virtio_net: can change MTU after installing program

[Toke Høiland-Jørgensen]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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
>> 
>> 
>
> Cc Toke who has tested this on other cards and has some input.

Well, my comment was just that we already restrict MTU changes on mlx5
when an XDP program is loaded:

$ sudo ip link set dev ens1f1 mtu 8192
RTNETLINK answers: Invalid argument

Reading through the rest of the thread I don't have any strong opinions
about whether this should propagate out from the host or not. I suspect
it would not be worth the trouble, though, and as you say it's already
possible to configure regular network devices in a way that is
incompatible with the rest of the network.

-Toke

Re: virtio_net: can change MTU after installing program

[David Ahern]

On 2/26/20 2:51 AM, Toke Høiland-Jørgensen wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
>> 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
>>>
>>>
>>
>> Cc Toke who has tested this on other cards and has some input.
> 
> Well, my comment was just that we already restrict MTU changes on mlx5
> when an XDP program is loaded:
> 
> $ sudo ip link set dev ens1f1 mtu 8192
> RTNETLINK answers: Invalid argument
> 
> Reading through the rest of the thread I don't have any strong opinions
> about whether this should propagate out from the host or not. I suspect
> it would not be worth the trouble, though, and as you say it's already
> possible to configure regular network devices in a way that is
> incompatible with the rest of the network.
> 

Both mlx5 and sfc restrict MTU change to XDP limits; virtio does not
which strikes me as a problem.

Re: virtio_net: can change MTU after installing program

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 09:03:47AM -0700, David Ahern wrote:
> On 2/26/20 2:51 AM, Toke Høiland-Jørgensen wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > 
> >> 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
> >>>
> >>>
> >>
> >> Cc Toke who has tested this on other cards and has some input.
> > 
> > Well, my comment was just that we already restrict MTU changes on mlx5
> > when an XDP program is loaded:
> > 
> > $ sudo ip link set dev ens1f1 mtu 8192
> > RTNETLINK answers: Invalid argument
> > 
> > Reading through the rest of the thread I don't have any strong opinions
> > about whether this should propagate out from the host or not. I suspect
> > it would not be worth the trouble, though, and as you say it's already
> > possible to configure regular network devices in a way that is
> > incompatible with the rest of the network.
> > 
> 
> Both mlx5 and sfc restrict MTU change to XDP limits; virtio does not
> which strikes me as a problem.

OK that seems to indicate an ndo callback as a reasonable way
to handle this. Right? The only problem is this might break
guests if they happen to reverse the order of
operations:
	1. set mtu
	2. detach xdp prog
would previously work fine, and would now give an error.

If we want to make it transparent for userspace,
I guess we can defer the actual update until xdp prog is detached.
Sound ugly and might still confuse some userspace ... worth it?

-- 
MST

Re: virtio_net: can change MTU after installing program

[David Ahern]

On 2/26/20 9:55 AM, Michael S. Tsirkin wrote:
> 
> OK that seems to indicate an ndo callback as a reasonable way
> to handle this. Right? The only problem is this might break
> guests if they happen to reverse the order of
> operations:
> 	1. set mtu
> 	2. detach xdp prog
> would previously work fine, and would now give an error.

That order should not work at all. You should not be allowed to change
the MTU size that exceeds XDP limits while an XDP program is attached.

> 
> If we want to make it transparent for userspace,
> I guess we can defer the actual update until xdp prog is detached.
> Sound ugly and might still confuse some userspace ... worth it?
> 

Re: virtio_net: can change MTU after installing program

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 09:58:00AM -0700, David Ahern wrote:
> On 2/26/20 9:55 AM, Michael S. Tsirkin wrote:
> > 
> > OK that seems to indicate an ndo callback as a reasonable way
> > to handle this. Right? The only problem is this might break
> > guests if they happen to reverse the order of
> > operations:
> > 	1. set mtu
> > 	2. detach xdp prog
> > would previously work fine, and would now give an error.
> 
> That order should not work at all. You should not be allowed to change
> the MTU size that exceeds XDP limits while an XDP program is attached.


Right. But we didn't check it so blocking that now is a UAPI change.
Do we care?

> > 
> > If we want to make it transparent for userspace,
> > I guess we can defer the actual update until xdp prog is detached.
> > Sound ugly and might still confuse some userspace ... worth it?
> > 

Re: virtio_net: can change MTU after installing program

[Jakub Kicinski]

On Wed, 26 Feb 2020 12:02:03 -0500 Michael S. Tsirkin wrote:
> On Wed, Feb 26, 2020 at 09:58:00AM -0700, David Ahern wrote:
> > On 2/26/20 9:55 AM, Michael S. Tsirkin wrote:  
> > > OK that seems to indicate an ndo callback as a reasonable way
> > > to handle this. Right? The only problem is this might break
> > > guests if they happen to reverse the order of
> > > operations:
> > > 	1. set mtu
> > > 	2. detach xdp prog
> > > would previously work fine, and would now give an error.  
> > 
> > That order should not work at all. You should not be allowed to change
> > the MTU size that exceeds XDP limits while an XDP program is attached.  
> 
> 
> Right. But we didn't check it so blocking that now is a UAPI change.
> Do we care?

I'd vote that we don't care. We should care more about consistency
across drivers than committing to buggy behavior.

All drivers should have this check (intel, mlx, nfp definitely do),
I had a look at Broadcom and it seems to be missing there as well :(
Qlogic also. Ugh.

Re: virtio_net: can change MTU after installing program

[Michael S. Tsirkin]

On Wed, Feb 26, 2020 at 05:37:51PM -0800, Jakub Kicinski wrote:
> On Wed, 26 Feb 2020 12:02:03 -0500 Michael S. Tsirkin wrote:
> > On Wed, Feb 26, 2020 at 09:58:00AM -0700, David Ahern wrote:
> > > On 2/26/20 9:55 AM, Michael S. Tsirkin wrote:  
> > > > OK that seems to indicate an ndo callback as a reasonable way
> > > > to handle this. Right? The only problem is this might break
> > > > guests if they happen to reverse the order of
> > > > operations:
> > > > 	1. set mtu
> > > > 	2. detach xdp prog
> > > > would previously work fine, and would now give an error.  
> > > 
> > > That order should not work at all. You should not be allowed to change
> > > the MTU size that exceeds XDP limits while an XDP program is attached.  
> > 
> > 
> > Right. But we didn't check it so blocking that now is a UAPI change.
> > Do we care?
> 
> I'd vote that we don't care. We should care more about consistency
> across drivers than committing to buggy behavior.
> 
> All drivers should have this check (intel, mlx, nfp definitely do),
> I had a look at Broadcom and it seems to be missing there as well :(
> Qlogic also. Ugh.

Any chance to put it in net core then? Seems straight-forward enough ...

-- 
MST

Re: virtio_net: can change MTU after installing program

[Jakub Kicinski]

On Thu, 27 Feb 2020 03:14:20 -0500 Michael S. Tsirkin wrote:
> On Wed, Feb 26, 2020 at 05:37:51PM -0800, Jakub Kicinski wrote:
> > On Wed, 26 Feb 2020 12:02:03 -0500 Michael S. Tsirkin wrote:  
> > I'd vote that we don't care. We should care more about consistency
> > across drivers than committing to buggy behavior.
> > 
> > All drivers should have this check (intel, mlx, nfp definitely do),
> > I had a look at Broadcom and it seems to be missing there as well :(
> > Qlogic also. Ugh.  
> 
> Any chance to put it in net core then? Seems straight-forward enough ...

It's not impossible, but generally the RX buf geometry requirements
are not universal (see ixgbe or nfp).

Re: virtio_net: can change MTU after installing program

[Michael Chan]

On Wed, Feb 26, 2020 at 5:37 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 26 Feb 2020 12:02:03 -0500 Michael S. Tsirkin wrote:
> > On Wed, Feb 26, 2020 at 09:58:00AM -0700, David Ahern wrote:
> > > On 2/26/20 9:55 AM, Michael S. Tsirkin wrote:
> > > > OK that seems to indicate an ndo callback as a reasonable way
> > > > to handle this. Right? The only problem is this might break
> > > > guests if they happen to reverse the order of
> > > > operations:
> > > >   1. set mtu
> > > >   2. detach xdp prog
> > > > would previously work fine, and would now give an error.
> > >
> > > That order should not work at all. You should not be allowed to change
> > > the MTU size that exceeds XDP limits while an XDP program is attached.
> >
> >
> > Right. But we didn't check it so blocking that now is a UAPI change.
> > Do we care?
>
> I'd vote that we don't care. We should care more about consistency
> across drivers than committing to buggy behavior.
>
> All drivers should have this check (intel, mlx, nfp definitely do),
> I had a look at Broadcom and it seems to be missing there as well :(
> Qlogic also. Ugh.

The Broadcom bnxt_en driver should not allow the MTU to be changed to
an invalid value after an XDP program is attached.  We set the
netdev->max_mtu to a smaller value and dev_validate_mtu() should
reject MTUs that are not supported in XDP mode.

Re: virtio_net: can change MTU after installing program

[Jakub Kicinski]

On Thu, 27 Feb 2020 11:26:58 -0800 Michael Chan wrote:
> On Wed, Feb 26, 2020 at 5:37 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > I had a look at Broadcom and it seems to be missing there as well :(
> > Qlogic also. Ugh.  
> 
> The Broadcom bnxt_en driver should not allow the MTU to be changed to
> an invalid value after an XDP program is attached.  We set the
> netdev->max_mtu to a smaller value and dev_validate_mtu() should
> reject MTUs that are not supported in XDP mode.

I see, thanks!

Re: virtio_net: can change MTU after installing program

[David Ahern]

On 2/27/20 12:26 PM, Michael Chan wrote:
> 
> The Broadcom bnxt_en driver should not allow the MTU to be changed to
> an invalid value after an XDP program is attached.  We set the
> netdev->max_mtu to a smaller value and dev_validate_mtu() should
> reject MTUs that are not supported in XDP mode.
> 

That's what I proposed for virtio_net. Simplest option and since max mtu
is now dumped with device details very easy for a user to determine why
a particular MTU fails.