diff mbox series

[PATCHv2,net-next,02/17] udp6: move the mss check after udp gso tunnel processing

Message ID c36b016ee429980b9585144f4f9af31bcda467ee.1602150362.git.lucien.xin@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series sctp: Implement RFC6951: UDP Encapsulation of SCTP | expand

Commit Message

Xin Long Oct. 8, 2020, 9:47 a.m. UTC 
For some protocol's gso, like SCTP, it's using GSO_BY_FRAGS for
gso_size. When using UDP to encapsulate its packet, it will
return error in udp6_ufo_fragment() as skb->len < gso_size,
and it will never go to the gso tunnel processing.

So we should move this check after udp gso tunnel processing,
the same as udp4_ufo_fragment() does. While at it, also tidy
the variables up.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv6/udp_offload.c | 154 ++++++++++++++++++++++++-------------------------
 1 file changed, 76 insertions(+), 78 deletions(-)

Comments

Willem de Bruijn Oct. 8, 2020, 12:44 p.m. UTC | #1 
On Thu, Oct 8, 2020 at 5:48 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> For some protocol's gso, like SCTP, it's using GSO_BY_FRAGS for
> gso_size. When using UDP to encapsulate its packet, it will
> return error in udp6_ufo_fragment() as skb->len < gso_size,
> and it will never go to the gso tunnel processing.
>
> So we should move this check after udp gso tunnel processing,
> the same as udp4_ufo_fragment() does. While at it, also tidy
> the variables up.

Please don't mix a new feature and code cleanup.

This patch changes almost every line of the function due to
indentation changes. But the only relevant part is

"
        mss = skb_shinfo(skb)->gso_size;
        if (unlikely(skb->len <= mss))
                goto out;

        if (skb->encapsulation && skb_shinfo(skb)->gso_type &
            (SKB_GSO_UDP_TUNNEL|SKB_GSO_UDP_TUNNEL_CSUM))
                segs = skb_udp_tunnel_segment(skb, features, true);
        else {
                /* irrelevant here */
        }

out:
        return segs;
}
"

Is it a sufficient change to just skip the mss check if mss == GSO_BY_FRAGS?
Xin Long Oct. 9, 2020, 1:48 a.m. UTC | #2 
On Thu, Oct 8, 2020 at 8:45 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> On Thu, Oct 8, 2020 at 5:48 AM Xin Long <lucien.xin@gmail.com> wrote:
> >
> > For some protocol's gso, like SCTP, it's using GSO_BY_FRAGS for
> > gso_size. When using UDP to encapsulate its packet, it will
> > return error in udp6_ufo_fragment() as skb->len < gso_size,
> > and it will never go to the gso tunnel processing.
> >
> > So we should move this check after udp gso tunnel processing,
> > the same as udp4_ufo_fragment() does. While at it, also tidy
> > the variables up.
>
> Please don't mix a new feature and code cleanup.
Hi, Willem,

Tidying up variables are not worth a single patch, that's what I was
thinking. I can leave the variables as it is if you wish in this patch.

>
> This patch changes almost every line of the function due to
> indentation changes. But the only relevant part is
>
> "
>         mss = skb_shinfo(skb)->gso_size;
>         if (unlikely(skb->len <= mss))
>                 goto out;
>
>         if (skb->encapsulation && skb_shinfo(skb)->gso_type &
>             (SKB_GSO_UDP_TUNNEL|SKB_GSO_UDP_TUNNEL_CSUM))
>                 segs = skb_udp_tunnel_segment(skb, features, true);
>         else {
>                 /* irrelevant here */
>         }
>
> out:
>         return segs;
> }
> "
>
> Is it a sufficient change to just skip the mss check if mss == GSO_BY_FRAGS?
It is sufficient.

But I think we'd better keep the code here consistent with ipv4's if
there's no other reason to do 'skb->len <= mss' check at the first.

We can go with if-else as you showed above now, then do a cleanup in
the future. What do you think?
Willem de Bruijn Oct. 9, 2020, 1:59 p.m. UTC | #3 
On Thu, Oct 8, 2020 at 9:49 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Thu, Oct 8, 2020 at 8:45 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > On Thu, Oct 8, 2020 at 5:48 AM Xin Long <lucien.xin@gmail.com> wrote:
> > >
> > > For some protocol's gso, like SCTP, it's using GSO_BY_FRAGS for
> > > gso_size. When using UDP to encapsulate its packet, it will
> > > return error in udp6_ufo_fragment() as skb->len < gso_size,
> > > and it will never go to the gso tunnel processing.
> > >
> > > So we should move this check after udp gso tunnel processing,
> > > the same as udp4_ufo_fragment() does. While at it, also tidy
> > > the variables up.
> >
> > Please don't mix a new feature and code cleanup.
> Hi, Willem,
>
> Tidying up variables are not worth a single patch, that's what I was
> thinking. I can leave the variables as it is if you wish in this patch.
>
> >
> > This patch changes almost every line of the function due to
> > indentation changes. But the only relevant part is
> >
> > "
> >         mss = skb_shinfo(skb)->gso_size;
> >         if (unlikely(skb->len <= mss))
> >                 goto out;
> >
> >         if (skb->encapsulation && skb_shinfo(skb)->gso_type &
> >             (SKB_GSO_UDP_TUNNEL|SKB_GSO_UDP_TUNNEL_CSUM))
> >                 segs = skb_udp_tunnel_segment(skb, features, true);
> >         else {
> >                 /* irrelevant here */
> >         }
> >
> > out:
> >         return segs;
> > }
> > "
> >
> > Is it a sufficient change to just skip the mss check if mss == GSO_BY_FRAGS?
> It is sufficient.
>
> But I think we'd better keep the code here consistent with ipv4's if
> there's no other reason to do 'skb->len <= mss' check at the first.
>
> We can go with if-else as you showed above now, then do a cleanup in
> the future. What do you think?

I do think that's a better approach, thanks.

Mixing in feature changes with 150+ lines of code refactoring makes it
hard to spot the behavioral change.

Too often the additional purportedly noop refactoring proves to have
subtle behavioral changes after all. It's easier to review for this if
that patch is separate. Not saying it's the case here, of course.

Refactoring also adds a layer of indirection when using git blame to
understand code or for people who have the existing code memorized, so
has a fairly high bar imho.
diff mbox series

Patch

diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 584157a..3c5ec8e 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -17,96 +17,94 @@ 
 static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 					 netdev_features_t features)
 {
+	u8 nexthdr, frag_hdr_sz = sizeof(struct frag_hdr);
+	unsigned int unfrag_ip6hlen, unfrag_len, mss;
 	struct sk_buff *segs = ERR_PTR(-EINVAL);
-	unsigned int mss;
-	unsigned int unfrag_ip6hlen, unfrag_len;
-	struct frag_hdr *fptr;
+	const struct ipv6hdr *ipv6h;
 	u8 *packet_start, *prevhdr;
-	u8 nexthdr;
-	u8 frag_hdr_sz = sizeof(struct frag_hdr);
+	struct frag_hdr *fptr;
+	int tnl_hlen, err;
+	struct udphdr *uh;
 	__wsum csum;
-	int tnl_hlen;
-	int err;
 
-	mss = skb_shinfo(skb)->gso_size;
-	if (unlikely(skb->len <= mss))
+	if (skb->encapsulation &&
+	    (skb_shinfo(skb)->gso_type &
+	     (SKB_GSO_UDP_TUNNEL | SKB_GSO_UDP_TUNNEL_CSUM))) {
+		segs = skb_udp_tunnel_segment(skb, features, true);
 		goto out;
+	}
 
-	if (skb->encapsulation && skb_shinfo(skb)->gso_type &
-	    (SKB_GSO_UDP_TUNNEL|SKB_GSO_UDP_TUNNEL_CSUM))
-		segs = skb_udp_tunnel_segment(skb, features, true);
-	else {
-		const struct ipv6hdr *ipv6h;
-		struct udphdr *uh;
+	if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_UDP | SKB_GSO_UDP_L4)))
+		goto out;
 
-		if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_UDP | SKB_GSO_UDP_L4)))
-			goto out;
+	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
+		goto out;
 
-		if (!pskb_may_pull(skb, sizeof(struct udphdr)))
-			goto out;
+	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
+		return __udp_gso_segment(skb, features);
 
-		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
-			return __udp_gso_segment(skb, features);
-
-		/* Do software UFO. Complete and fill in the UDP checksum as HW cannot
-		 * do checksum of UDP packets sent as multiple IP fragments.
-		 */
-
-		uh = udp_hdr(skb);
-		ipv6h = ipv6_hdr(skb);
-
-		uh->check = 0;
-		csum = skb_checksum(skb, 0, skb->len, 0);
-		uh->check = udp_v6_check(skb->len, &ipv6h->saddr,
-					  &ipv6h->daddr, csum);
-		if (uh->check == 0)
-			uh->check = CSUM_MANGLED_0;
-
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-
-		/* If there is no outer header we can fake a checksum offload
-		 * due to the fact that we have already done the checksum in
-		 * software prior to segmenting the frame.
-		 */
-		if (!skb->encap_hdr_csum)
-			features |= NETIF_F_HW_CSUM;
-
-		/* Check if there is enough headroom to insert fragment header. */
-		tnl_hlen = skb_tnl_header_len(skb);
-		if (skb->mac_header < (tnl_hlen + frag_hdr_sz)) {
-			if (gso_pskb_expand_head(skb, tnl_hlen + frag_hdr_sz))
-				goto out;
-		}
+	mss = skb_shinfo(skb)->gso_size;
+	if (unlikely(skb->len <= mss))
+		goto out;
 
-		/* Find the unfragmentable header and shift it left by frag_hdr_sz
-		 * bytes to insert fragment header.
-		 */
-		err = ip6_find_1stfragopt(skb, &prevhdr);
-		if (err < 0)
-			return ERR_PTR(err);
-		unfrag_ip6hlen = err;
-		nexthdr = *prevhdr;
-		*prevhdr = NEXTHDR_FRAGMENT;
-		unfrag_len = (skb_network_header(skb) - skb_mac_header(skb)) +
-			     unfrag_ip6hlen + tnl_hlen;
-		packet_start = (u8 *) skb->head + SKB_GSO_CB(skb)->mac_offset;
-		memmove(packet_start-frag_hdr_sz, packet_start, unfrag_len);
-
-		SKB_GSO_CB(skb)->mac_offset -= frag_hdr_sz;
-		skb->mac_header -= frag_hdr_sz;
-		skb->network_header -= frag_hdr_sz;
-
-		fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
-		fptr->nexthdr = nexthdr;
-		fptr->reserved = 0;
-		fptr->identification = ipv6_proxy_select_ident(dev_net(skb->dev), skb);
-
-		/* Fragment the skb. ipv6 header and the remaining fields of the
-		 * fragment header are updated in ipv6_gso_segment()
-		 */
-		segs = skb_segment(skb, features);
+	/* Do software UFO. Complete and fill in the UDP checksum as HW cannot
+	 * do checksum of UDP packets sent as multiple IP fragments.
+	 */
+
+	uh = udp_hdr(skb);
+	ipv6h = ipv6_hdr(skb);
+
+	uh->check = 0;
+	csum = skb_checksum(skb, 0, skb->len, 0);
+	uh->check = udp_v6_check(skb->len, &ipv6h->saddr,
+				 &ipv6h->daddr, csum);
+	if (uh->check == 0)
+		uh->check = CSUM_MANGLED_0;
+
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	/* If there is no outer header we can fake a checksum offload
+	 * due to the fact that we have already done the checksum in
+	 * software prior to segmenting the frame.
+	 */
+	if (!skb->encap_hdr_csum)
+		features |= NETIF_F_HW_CSUM;
+
+	/* Check if there is enough headroom to insert fragment header. */
+	tnl_hlen = skb_tnl_header_len(skb);
+	if (skb->mac_header < (tnl_hlen + frag_hdr_sz)) {
+		if (gso_pskb_expand_head(skb, tnl_hlen + frag_hdr_sz))
+			goto out;
 	}
 
+	/* Find the unfragmentable header and shift it left by frag_hdr_sz
+	 * bytes to insert fragment header.
+	 */
+	err = ip6_find_1stfragopt(skb, &prevhdr);
+	if (err < 0)
+		return ERR_PTR(err);
+	unfrag_ip6hlen = err;
+	nexthdr = *prevhdr;
+	*prevhdr = NEXTHDR_FRAGMENT;
+	unfrag_len = (skb_network_header(skb) - skb_mac_header(skb)) +
+		     unfrag_ip6hlen + tnl_hlen;
+	packet_start = (u8 *)skb->head + SKB_GSO_CB(skb)->mac_offset;
+	memmove(packet_start - frag_hdr_sz, packet_start, unfrag_len);
+
+	SKB_GSO_CB(skb)->mac_offset -= frag_hdr_sz;
+	skb->mac_header -= frag_hdr_sz;
+	skb->network_header -= frag_hdr_sz;
+
+	fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
+	fptr->nexthdr = nexthdr;
+	fptr->reserved = 0;
+	fptr->identification = ipv6_proxy_select_ident(dev_net(skb->dev), skb);
+
+	/* Fragment the skb. ipv6 header and the remaining fields of the
+	 * fragment header are updated in ipv6_gso_segment()
+	 */
+	segs = skb_segment(skb, features);
+
 out:
 	return segs;
 }