[PATCH net] udp: segment looped gso packets correctly

[PATCH net] udp: segment looped gso packets correctly

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Multicast and broadcast packets can be looped from egress to ingress
pre segmentation with dev_loopback_xmit. That function unconditionally
sets ip_summed to CHECKSUM_UNNECESSARY.

udp_rcv_segment segments gso packets in the udp rx path. Segmentation
usually executes on egress, and does not expect packets of this type.
__udp_gso_segment interprets !CHECKSUM_PARTIAL as CHECKSUM_NONE. But
the offsets are not correct for gso_make_checksum.

UDP GSO packets are of type CHECKSUM_PARTIAL, with their uh->check set
to the correct pseudo header checksum. Reset ip_summed to this type.
(CHECKSUM_PARTIAL is allowed on ingress, see comments in skbuff.h)

Reported-by: syzbot <syzkaller@googlegroups.com>
Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 include/net/udp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/net/udp.h b/include/net/udp.h
index bad74f7808311..8f163d674f072 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -476,6 +476,9 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
 	if (!inet_get_convert_csum(sk))
 		features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 
+	if (skb->pkt_type == PACKET_LOOPBACK)
+		skb->ip_summed = CHECKSUM_PARTIAL;
+
 	/* the GSO CB lays after the UDP one, no need to save and restore any
 	 * CB fragment
 	 */
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH net] udp: segment looped gso packets correctly

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Mon, 27 Jan 2020 15:40:31 -0500

> From: Willem de Bruijn <willemb@google.com>
> 
> Multicast and broadcast packets can be looped from egress to ingress
> pre segmentation with dev_loopback_xmit. That function unconditionally
> sets ip_summed to CHECKSUM_UNNECESSARY.
> 
> udp_rcv_segment segments gso packets in the udp rx path. Segmentation
> usually executes on egress, and does not expect packets of this type.
> __udp_gso_segment interprets !CHECKSUM_PARTIAL as CHECKSUM_NONE. But
> the offsets are not correct for gso_make_checksum.
> 
> UDP GSO packets are of type CHECKSUM_PARTIAL, with their uh->check set
> to the correct pseudo header checksum. Reset ip_summed to this type.
> (CHECKSUM_PARTIAL is allowed on ingress, see comments in skbuff.h)
> 
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Applied and queued up for -stable, but I have to say:

> +	if (skb->pkt_type == PACKET_LOOPBACK)
> +		skb->ip_summed = CHECKSUM_PARTIAL;
> +

There are a lot of implementation detail assumptions encoded into that
conditional statement :-)

Feel free to follow-up with a patch adding a comment containing a
condensed version of your commit log here.

Thanks.

Re: [PATCH net] udp: segment looped gso packets correctly

[Paolo Abeni]

On Mon, 2020-01-27 at 15:40 -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Multicast and broadcast packets can be looped from egress to ingress
> pre segmentation with dev_loopback_xmit. That function unconditionally
> sets ip_summed to CHECKSUM_UNNECESSARY.
> 
> udp_rcv_segment segments gso packets in the udp rx path. Segmentation
> usually executes on egress, and does not expect packets of this type.
> __udp_gso_segment interprets !CHECKSUM_PARTIAL as CHECKSUM_NONE. But
> the offsets are not correct for gso_make_checksum.
> 
> UDP GSO packets are of type CHECKSUM_PARTIAL, with their uh->check set
> to the correct pseudo header checksum. Reset ip_summed to this type.
> (CHECKSUM_PARTIAL is allowed on ingress, see comments in skbuff.h)
> 
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  include/net/udp.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/net/udp.h b/include/net/udp.h
> index bad74f7808311..8f163d674f072 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -476,6 +476,9 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
>  	if (!inet_get_convert_csum(sk))
>  		features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>  
> +	if (skb->pkt_type == PACKET_LOOPBACK)
> +		skb->ip_summed = CHECKSUM_PARTIAL;
> +
>  	/* the GSO CB lays after the UDP one, no need to save and restore any
>  	 * CB fragment
>  	 */

LGTM, Thanks!

Acked-by: Paolo Abeni <pabeni@redhat.com>

Out of sheer curiosity, do you know what was the kernel behaviour
before the 'fixes' commit ? GSO packet delivered to user-space stillaggregated ?!? 

Thanks,

Paolo

Re: [PATCH net] udp: segment looped gso packets correctly

[Willem de Bruijn]

On Tue, Jan 28, 2020 at 6:27 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2020-01-27 at 15:40 -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Multicast and broadcast packets can be looped from egress to ingress
> > pre segmentation with dev_loopback_xmit. That function unconditionally
> > sets ip_summed to CHECKSUM_UNNECESSARY.
> >
> > udp_rcv_segment segments gso packets in the udp rx path. Segmentation
> > usually executes on egress, and does not expect packets of this type.
> > __udp_gso_segment interprets !CHECKSUM_PARTIAL as CHECKSUM_NONE. But
> > the offsets are not correct for gso_make_checksum.
> >
> > UDP GSO packets are of type CHECKSUM_PARTIAL, with their uh->check set
> > to the correct pseudo header checksum. Reset ip_summed to this type.
> > (CHECKSUM_PARTIAL is allowed on ingress, see comments in skbuff.h)
> >
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> > ---
> >  include/net/udp.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/net/udp.h b/include/net/udp.h
> > index bad74f7808311..8f163d674f072 100644
> > --- a/include/net/udp.h
> > +++ b/include/net/udp.h
> > @@ -476,6 +476,9 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> >       if (!inet_get_convert_csum(sk))
> >               features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> >
> > +     if (skb->pkt_type == PACKET_LOOPBACK)
> > +             skb->ip_summed = CHECKSUM_PARTIAL;
> > +
> >       /* the GSO CB lays after the UDP one, no need to save and restore any
> >        * CB fragment
> >        */
>
> LGTM, Thanks!
>
> Acked-by: Paolo Abeni <pabeni@redhat.com>
>
> Out of sheer curiosity, do you know what was the kernel behaviour
> before the 'fixes' commit ? GSO packet delivered to user-space stillaggregated ?!?

Yes.. I had missed this entire ip_mc_output path when adding
segmentation (clearly).

Your patch fixed that. The other options would be segmenting in
dev_loopback_xmit or outright failing the call in udp_send_skb.
Neither of which is appealing.

Thanks for reviewing!

Re: [PATCH net] udp: segment looped gso packets correctly

[Willem de Bruijn]

On Wed, Jan 29, 2020 at 11:58 AM David Miller <davem@davemloft.net> wrote:
>
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Mon, 27 Jan 2020 15:40:31 -0500
>
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Multicast and broadcast packets can be looped from egress to ingress
> > pre segmentation with dev_loopback_xmit. That function unconditionally
> > sets ip_summed to CHECKSUM_UNNECESSARY.
> >
> > udp_rcv_segment segments gso packets in the udp rx path. Segmentation
> > usually executes on egress, and does not expect packets of this type.
> > __udp_gso_segment interprets !CHECKSUM_PARTIAL as CHECKSUM_NONE. But
> > the offsets are not correct for gso_make_checksum.
> >
> > UDP GSO packets are of type CHECKSUM_PARTIAL, with their uh->check set
> > to the correct pseudo header checksum. Reset ip_summed to this type.
> > (CHECKSUM_PARTIAL is allowed on ingress, see comments in skbuff.h)
> >
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Fixes: cf329aa42b66 ("udp: cope with UDP GRO packet misdirection")
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> Applied and queued up for -stable, but I have to say:
>
> > +     if (skb->pkt_type == PACKET_LOOPBACK)
> > +             skb->ip_summed = CHECKSUM_PARTIAL;
> > +
>
> There are a lot of implementation detail assumptions encoded into that
> conditional statement :-)
>
> Feel free to follow-up with a patch adding a comment containing a
> condensed version of your commit log here.

Will do. Yeah, that does look pretty obscure. I may have gotten a bit
too used to relying solely on git blame ;-)

Thanks!