Date: Thu, 11 Apr 2024 13:53:32 +0800
From: Jason Wang <jasowang@...hat.com>
To: Yuri Benditovich <yuri.benditovich@...nix.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, yan@...nix.com, 
	andrew@...nix.com, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: change maximum number of UDP segments to 128

On Thu, Apr 11, 2024 at 1:32 PM Yuri Benditovich
<yuri.benditovich@...nix.com> wrote:
>
> On Thu, Apr 11, 2024 at 7:04 AM Jason Wang <jasowang@...hat.com> wrote:
> >
> > On Wed, Apr 10, 2024 at 4:28 PM Yuri Benditovich
> > <yuri.benditovich@...nix.com> wrote:
> > >
> > > On Wed, Apr 10, 2024 at 9:07 AM Jason Wang <jasowang@...hat.com> wrote:
> > > >
> > > > On Tue, Apr 9, 2024 at 1:48 PM Yuri Benditovich
> > > > <yuri.benditovich@...nix.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Apr 9, 2024 at 6:53 AM Jason Wang <jasowang@...hat.com> wrote:
> > > > >>
> > > > >> On Mon, Apr 8, 2024 at 3:24 PM Yuri Benditovich
> > > > >> <yuri.benditovich@...nix.com> wrote:
> > > > >> >
> > > > >> > On Mon, Apr 8, 2024 at 9:27 AM Jason Wang <jasowang@...hat.com> wrote:
> > > > >> > >
> > > > >> > > On Sun, Apr 7, 2024 at 2:50 AM Yuri Benditovich
> > > > >> > > <yuri.benditovich@...nix.com> wrote:
> > > > >> > > >
> > > > >> > > > Fixes: fc8b2a619469378 ("net: more strict VIRTIO_NET_HDR_GSO_UDP_L4 validation")
> > > > >> > > >
> > > > >> > > > The mentioned above commit adds check of potential number
> > > > >> > > > of UDP segments vs UDP_MAX_SEGMENTS in linux/virtio_net.h.
> > > > >> > > > After this change certification test of USO guest-to-guest
> > > > >> > > > transmit on Windows driver for virtio-net device fails,
> > > > >> > > > for example with packet size of ~64K and mss of 536 bytes.
> > > > >> > > > In general the USO should not be more restrictive than TSO.
> > > > >> > > > Indeed, in case of unreasonably small mss a lot of segments
> > > > >> > > > can cause queue overflow and packet loss on the destination.
> > > > >> > > > Limit of 128 segments is good for any practical purpose,
> > > > >> > > > with minimal meaningful mss of 536 the maximal UDP packet will
> > > > >> > > > be divided to ~120 segments.
> > > > >> > >
> > > > >> > > Assuming different OS guests could run on top of KVM. I wonder if a
> > > > >> > > better fix is to relax the following check:
> > > > >> > >
> > > > >> > > =>                      if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
> > > > >> > >                                 return -EINVAL;
> > > > >> > There are also checks vs UDP_MAX_SEGMENTS in udp.c (in ipv4 and ipv6),
> > > > >> > they do not prevent guest-to-guest from passing but cause packet dropping in
> > > > >> > other cases..
> > > > >> > >
> > > > >> > > Changing UDP_MAX_SEGMENTS may have side effects. For example, a new
> > > > >> > > Linux guest run on top of a old Linux and other.
> > > > >> > IMO, in the worst case _in specific setups_ the communication will behave like
> > > > >> > it does now in all the setups.
> > > > >>
> > > > >> I meant if the guest limit is 128 but host limit is 64.
> > > > >
> > > > >
> > > > > If the guest limit is (128 or 64) and host limit is 64:
> > > > > If we send a UDP packet  with USO (length 64K, mss 600) - it is dropped.
> > > > > setsockopt does not limit us to use <=64 packets, neither in windows nor in Linux,
> > > >
> > > > Just to make sure we are on the same page:
> > > >
> > > > Before fc8b2a619469378,
> > > >
> > > > 1) Windows guest works on Linux Host since we don't check against
> > > > UDP_MAX_SEGMENTS on the host
> > >
> > >
> > > Windows guest-to-guest works since there  is no check for
> > > UDP_MAX_SEGMENTS in virtio_net.h
> > > Windows guest to Linux host suffers with size=64K mss=536 (as an example)
> > >
> > > > 2) Linux guest works on Linux Host, since it will always send packet
> > > > with less than UDP_MAX_SEGMENTS
> > >
> > >
> > > Not exactly.
> > > If you use "selftest" (udpgso*) _as is_, it will work as it has
> > > _internal_ define of 64 and never tries to do more than that.
> > > But (sorry for repeating that) there is no setsockopt() limitation and
> > > you're free to modify the test code
> > > to use more segments (as an example) and this will not work.
> >
> > I meant this part in udp_send_skb()
> >
> > =>              if (datalen > cork->gso_size * UDP_MAX_SEGMENTS) {
> >                         kfree_skb(skb);
> >                         return -EINVAL;
> >                 }
> >
> > >
> > > >
> > > >
> > > > This is the behaviour that we need to stick to, otherwise we break the
> > > > guests as you've noticed.
> > >
> > > We can stick to the behavior I've described above if we change only define
> > > UDP_MAX_SEGMENTS 64->128 in udp.h but leave untouched
> > > UDP_MAX_SEGMENTS=64 in udpgso (test code)
> > > (IMO does not make too much sense but possible)
> > >
> > > Actually, all this discussion happens because the initial code of USO
> > > in the kernel was delivered with define of 64 without any special
> > > reason, just because this looked enough then.
> >
> > Right, but it's too late to "fix", the only thing we can do now is to
> > make sure to not break the application that worked in the past.
> >
> > >
> > > >
> > > > If we fix it by increasing the UDP_MAX_SEGMENTS, it fixes for 1) but
> > > > not necessarily for 2). If we don't check against UDP_MAX_SEGMENTS, it
> > > > works for 2) as well.
> > >
> >
> > So what I want to say is that having a check for UDP_MAX_SEGMENTS
> > seems to be problematic. And increasing it to 128 complicates the
> > problem furtherly.
> >
> > > There are following cases:
> > > - guest-to-guest (win-to-win, lin-to-lin,win-to-lin, lin-to-win)
> >
> > If we don't have the check for UDP_MAX_SEGMENTS, everything should be fine.
> >
> > If the sender can produces 128 and 64 is checked in the middle of the
> > datapath in either tun or virtio-net, the packet will be dropped.
> >
> > > -guest-to-host (guest=lin, guest=win)
> >
> > Same as above.
> >
> > > -host-to-host
> >
> > I don't see any issue in this part.
>
> The issue is that host-to-host UDP packets with more than 64 segments
> are dropped due to check vs  UDP_MAX_SEGMENTS is udp.c (v4 and v6)

Yes, but if they fail since the introduction of UDP gso if I was not wrong.

>
>
> >
> > > -host-to-guest (guest=lin, guest=win)
> >
> > Same as guest-to-guest.
> >
> > >
> > > Resulting table of results is much more complicated than you described.
> > > We can't use the udpgso code as the only indicator of go/nogo.
> >
> > We should not break userspace. If there's a setup that udpgso can work
> > in the past but not after a patch is applied, we should avoid such
> > cases.
> >
> > That's why I think avoiding checking UDP_MAX_SEGMENTS and doing other
> > hardening might be better (for example as Eric suggested).
>
> I've checked all this thread and I did not find any Eric's suggestion regarding
> other hardening. Which suggestion have you mentioned?

Quoted from Eric email (btw, for some reason the discussion is not
reached to the list, adding list here)

"""

> Assuming different OS guests could run on top of KVM. I wonder if a
> better fix is to relax the following check:
>
> =>                      if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
>                                 return -EINVAL;
>
> Changing UDP_MAX_SEGMENTS may have side effects. For example, a new
> Linux guest run on top of a old Linux and other.

Typical qdisc packet limit is 1000.

I think a limit prevents abuses, like gso_size == 1, and skb->len = 65000

"""

The actual value needs more thought, for example 65K may block the
implementation of jumbogram in the future.

Thanks

>
> > >
> > > >
> > > > >
> > > > >>
> > > > >>
> > > > >> > Note that this limit (UDP_MAX_SEGMENTS) is set
> > > > >> > in internal kernel define, it is not based on any RFC, not visible via
> > > > >> > ethtool etc.
> > > > >>
> > > > >> In the future, it needs to be advertised to the guest via virtio spec.
> > > > >> Otherwise it might have compatibility issues.
> > > > >
> > > > >
> > > > > The spec targets 2 areas - hardware solutions and software solutions.
> > > > > For hardware solutions everything is clear - there are no declared limits and if the hardware will limit the number of segments to anything - it is a bug.
> > > > > So the advertising makes sense only for hypervisors and for example, qemu will need to obtain this magic number from the kernel.
> > > >
> > > > Right.
> > > >
> > > > > If/when we'll start adding this to the spec we'll face a reasonable question - why do we need to add something that is not mentioned in any RFC, why not add such a thing for TCP also?
> > > >
> > > > So the kernel has gso_max_segs for netdevice. We probably need it for
> > > > virtio-net as well, and it might be useful for jumbograms as well.
> > > >
> > > virtio-net is "limited" by common "#define GSO_MAX_SEGS        65535u"
> >
> > This works since the host will do software segmentation as a fallback
> > which will be very slow.
> >
> > >
> > > >
> > > > >
> > > > > Next, let's say we advertise this number over virtio capabilities - does this solve something?
> > > >
> > > > For example, doing software segmentation by kernel?
> > > >
> > > > > The driver can't communicate this capability up. The setsockopt will not limit this parameter.
> > > > >
> > > > >>
> > > > >> For example, migrate from 64 to 128.
> > > > >>
> > > > > Yes, but the migration from higher kernel to lower one looks like an immortal problem.
> > > >
> > > > This is not rare. For example, the patch for 128 is applied on the
> > > > source but not destination.
> > >
> > > I agree that such migration can happen and _theoretically_ may cause
> > > some misunderstanding
> > > I'm almost sure that practically this can't cause any.misunderstanding
> > > and we can discuss this specific problem in depth.
> > > (I just need some time for some tests)
> >
> > Thanks
> >
> > >
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > >>
> > > > >> > The same on Windows - the adapter does not have such a capability as maximal
> > > > >> > number of UDP segments, so the upper layers are unaware of any limitation.
> > > > >>
> > > > >> Right, that might be problematic.
> > > > >>
> > > > >> The checking of UDP_MAX_SEGMENTS implies an agreement of guest and
> > > > >> host.  But such implications are not true  .
> > > > >>
> > > > >> Thanks
> > > > >>
> > > > >>
> > > > >>
> > > > >> > >
> > > > >> > > Thanks
> > > > >> > >
> > > > >> > > >
> > > > >> > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@...nix.com>
> > > > >> > > > ---
> > > > >> > > >  include/linux/udp.h                  | 2 +-
> > > > >> > > >  tools/testing/selftests/net/udpgso.c | 2 +-
> > > > >> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > >> > > >
> > > > >> > > > diff --git a/include/linux/udp.h b/include/linux/udp.h
> > > > >> > > > index 3748e82b627b..7e75ccdf25fe 100644
> > > > >> > > > --- a/include/linux/udp.h
> > > > >> > > > +++ b/include/linux/udp.h
> > > > >> > > > @@ -108,7 +108,7 @@ struct udp_sock {
> > > > >> > > >  #define udp_assign_bit(nr, sk, val)            \
> > > > >> > > >         assign_bit(UDP_FLAGS_##nr, &udp_sk(sk)->udp_flags, val)
> > > > >> > > >
> > > > >> > > > -#define UDP_MAX_SEGMENTS       (1 << 6UL)
> > > > >> > > > +#define UDP_MAX_SEGMENTS       (1 << 7UL)
> > > > >> > > >
> > > > >> > > >  #define udp_sk(ptr) container_of_const(ptr, struct udp_sock, inet.sk)
> > > > >> > > >
> > > > >> > > > diff --git a/tools/testing/selftests/net/udpgso.c b/tools/testing/selftests/net/udpgso.c
> > > > >> > > > index 1d975bf52af3..85b3baa3f7f3 100644
> > > > >> > > > --- a/tools/testing/selftests/net/udpgso.c
> > > > >> > > > +++ b/tools/testing/selftests/net/udpgso.c
> > > > >> > > > @@ -34,7 +34,7 @@
> > > > >> > > >  #endif
> > > > >> > > >
> > > > >> > > >  #ifndef UDP_MAX_SEGMENTS
> > > > >> > > > -#define UDP_MAX_SEGMENTS       (1 << 6UL)
> > > > >> > > > +#define UDP_MAX_SEGMENTS       (1 << 7UL)
> > > > >> > > >  #endif
> > > > >> > > >
> > > > >> > > >  #define CONST_MTU_TEST 1500
> > > > >> > > > --
> > > > >> > > > 2.34.3
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > >
> > >
> >
>

Powered by blists - more mailing lists