On Mon, 2021-03-22 at 09:42 -0400, Willem de Bruijn wrote:
> On Sun, Mar 21, 2021 at 1:01 PM Paolo Abeni <pab...@redhat.com> wrote:
> > Currently the UDP protocol delivers GSO_FRAGLIST packets to
> > the sockets without the expected segmentation.
> >
> > This change addresses the issue introducing and maintaining
> > a per socket bitmask of GSO types requiring segmentation.
> > Enabling GSO removes SKB_GSO_UDP_L4 from such mask, while
> > GSO_FRAGLIST packets are never accepted
> >
> > Note: this also updates the 'unused' field size to really
> > fit the otherwise existing hole. It's size become incorrect
> > after commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").
> >
> > Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
> > Signed-off-by: Paolo Abeni <pab...@redhat.com>
> > ---
> > include/linux/udp.h | 10 ++++++----
> > net/ipv4/udp.c | 12 +++++++++++-
> > 2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/udp.h b/include/linux/udp.h
> > index aa84597bdc33c..6da342f15f351 100644
> > --- a/include/linux/udp.h
> > +++ b/include/linux/udp.h
> > @@ -51,7 +51,7 @@ struct udp_sock {
> > * different encapsulation layer
> > set
> > * this
> > */
> > - gro_enabled:1; /* Can accept GRO packets */
> > + gro_enabled:1; /* Request GRO aggregation */
>
> unnecessary comment change?
Before this patch 'gro_enabled' was used in udp_unexpected_gso() to
check for GSO packets acceptance, after this patch such field is not
used there anymore, so does not carry explicilty the 'accept GRO
packets' semantic.
Anyway I don't have strong feeling regarding changing or not this
comment
> > /*
> > * Following member retains the information to create a UDP header
> > * when the socket is uncorked.
> > @@ -68,7 +68,10 @@ struct udp_sock {
> > #define UDPLITE_SEND_CC 0x2 /* set via udplite setsockopt
> > */
> > #define UDPLITE_RECV_CC 0x4 /* set via udplite setsocktopt
> > */
> > __u8 pcflag; /* marks socket as UDP-Lite if > 0
> > */
> > - __u8 unused[3];
> > + __u8 unused[1];
> > + unsigned int unexpected_gso;/* GSO types this socket can't
> > accept,
> > + * any of SKB_GSO_UDP_L4 or
> > SKB_GSO_FRAGLIST
> > + */
>
> An extra unsigned int for this seems overkill.
Should be more clear after the next patch.
Using an explicit 'acceptable GSO types' field makes the patch 5/8
quite simple.
After this patch the 'udp_sock' struct size remains unchanged and even
the number of 'udp_sock' cachelines touched for every packet is
unchanged.
I opted for an 'unsigned int' so that I could simply copy a gso_type
there.
> Current sockets that support SKB_GSO_UDP_L4 implicitly also support
> SKB_GSO_FRAGLIST. This patch makes explicit that the second is not
> supported..
>
> > /*
> > * For encapsulation sockets.
> > */
> > @@ -131,8 +134,7 @@ static inline void udp_cmsg_recv(struct msghdr *msg,
> > struct sock *sk,
> >
> > static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff *skb)
> > {
> > - return !udp_sk(sk)->gro_enabled && skb_is_gso(skb) &&
> > - skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4;
> > + return skb_is_gso(skb) && skb_shinfo(skb)->gso_type &
> > udp_sk(sk)->unexpected_gso;
>
> .. just update this function as follows ?
>
> - return !udp_sk(sk)->gro_enabled && skb_is_gso(skb) &&
> - skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4;
> + return skb_is_gso(skb) &&
> + (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST ||
> !udp_sk(sk)->gro_enabled)
>
> where the latter is shorthand for
>
> (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && !udp_sk(sk)->gro_enabled)
>
> but the are the only two GSO types that could arrive here.
With the above patch 5/8 becomes messy ?!?
Thanks!
Paolo
