[PATCH 1/2] udp: fix gso_segs calculations

[PATCH 1/2] udp: fix gso_segs calculations

[Josh Hunt]

Commit dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
added gso_segs calculation, but incorrectly got sizeof() the pointer and
not the underlying data type. It also does not account for v6 UDP GSO segs.

Fixes: dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 net/ipv4/udp.c | 2 +-
 net/ipv6/udp.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index cf755156a684..be98d0b8f014 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -856,7 +856,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 
 		skb_shinfo(skb)->gso_size = cork->gso_size;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
-		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
+		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
 							 cork->gso_size);
 		goto csum_partial;
 	}
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index aae4938f3dea..eb9a9934ac05 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1143,6 +1143,8 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 
 		skb_shinfo(skb)->gso_size = cork->gso_size;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
+		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
+							 cork->gso_size);
 		goto csum_partial;
 	}
 
-- 
2.7.4

[PATCH 2/2] udp: only do GSO if # of segs > 1

[Josh Hunt]

Prior to this change an application sending <= 1MSS worth of data and
enabling UDP GSO would fail if the system had SW GSO enabled, but the
same send would succeed if HW GSO offload is enabled. In addition to this
inconsistency the error in the SW GSO case does not get back to the
application if sending out of a real device so the user is unaware of this
failure.

With this change we only perform GSO if the # of segments is > 1 even
if the application has enabled segmentation. I've also updated the
relevant udpgso selftests.

Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 net/ipv4/udp.c                       |  5 +++--
 net/ipv6/udp.c                       |  5 +++--
 tools/testing/selftests/net/udpgso.c | 16 ++++------------
 3 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index be98d0b8f014..ac0baf947560 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -821,6 +821,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 	int is_udplite = IS_UDPLITE(sk);
 	int offset = skb_transport_offset(skb);
 	int len = skb->len - offset;
+	int datalen = len - sizeof(*uh);
 	__wsum csum = 0;
 
 	/*
@@ -832,7 +833,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 	uh->len = htons(len);
 	uh->check = 0;
 
-	if (cork->gso_size) {
+	if (cork->gso_size && datalen > cork->gso_size) {
 		const int hlen = skb_network_header_len(skb) +
 				 sizeof(struct udphdr);
 
@@ -856,7 +857,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 
 		skb_shinfo(skb)->gso_size = cork->gso_size;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
-		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
+		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
 							 cork->gso_size);
 		goto csum_partial;
 	}
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index eb9a9934ac05..5e5f4013d905 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1109,6 +1109,7 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 	__wsum csum = 0;
 	int offset = skb_transport_offset(skb);
 	int len = skb->len - offset;
+	int datalen = len - sizeof(*uh);
 
 	/*
 	 * Create a UDP header
@@ -1119,7 +1120,7 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 	uh->len = htons(len);
 	uh->check = 0;
 
-	if (cork->gso_size) {
+	if (cork->gso_size && datalen > cork->gso_size) {
 		const int hlen = skb_network_header_len(skb) +
 				 sizeof(struct udphdr);
 
@@ -1143,7 +1144,7 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
 
 		skb_shinfo(skb)->gso_size = cork->gso_size;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
-		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
+		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(datalen,
 							 cork->gso_size);
 		goto csum_partial;
 	}
diff --git a/tools/testing/selftests/net/udpgso.c b/tools/testing/selftests/net/udpgso.c
index b8265ee9923f..614b31aad168 100644
--- a/tools/testing/selftests/net/udpgso.c
+++ b/tools/testing/selftests/net/udpgso.c
@@ -89,12 +89,9 @@ struct testcase testcases_v4[] = {
 		.tfail = true,
 	},
 	{
-		/* send a single MSS: will fail with GSO, because the segment
-		 * logic in udp4_ufo_fragment demands a gso skb to be > MTU
-		 */
+		/* send a single MSS: will fall back to no GSO */
 		.tlen = CONST_MSS_V4,
 		.gso_len = CONST_MSS_V4,
-		.tfail = true,
 		.r_num_mss = 1,
 	},
 	{
@@ -139,10 +136,9 @@ struct testcase testcases_v4[] = {
 		.tfail = true,
 	},
 	{
-		/* send a single 1B MSS: will fail, see single MSS above */
+		/* send a single 1B MSS: will fall back to no GSO */
 		.tlen = 1,
 		.gso_len = 1,
-		.tfail = true,
 		.r_num_mss = 1,
 	},
 	{
@@ -196,12 +192,9 @@ struct testcase testcases_v6[] = {
 		.tfail = true,
 	},
 	{
-		/* send a single MSS: will fail with GSO, because the segment
-		 * logic in udp4_ufo_fragment demands a gso skb to be > MTU
-		 */
+		/* send a single MSS: will fall back to no GSO */
 		.tlen = CONST_MSS_V6,
 		.gso_len = CONST_MSS_V6,
-		.tfail = true,
 		.r_num_mss = 1,
 	},
 	{
@@ -246,10 +239,9 @@ struct testcase testcases_v6[] = {
 		.tfail = true,
 	},
 	{
-		/* send a single 1B MSS: will fail, see single MSS above */
+		/* send a single 1B MSS: will fall back to no GSO */
 		.tlen = 1,
 		.gso_len = 1,
-		.tfail = true,
 		.r_num_mss = 1,
 	},
 	{
-- 
2.7.4

Re: [PATCH 1/2] udp: fix gso_segs calculations

[Alexander Duyck]

On Mon, Sep 30, 2019 at 3:15 PM Josh Hunt <johunt@akamai.com> wrote:
>
> Commit dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
> added gso_segs calculation, but incorrectly got sizeof() the pointer and
> not the underlying data type. It also does not account for v6 UDP GSO segs.
>
> Fixes: dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
> Signed-off-by: Josh Hunt <johunt@akamai.com>
> ---
>  net/ipv4/udp.c | 2 +-
>  net/ipv6/udp.c | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index cf755156a684..be98d0b8f014 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -856,7 +856,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>
>                 skb_shinfo(skb)->gso_size = cork->gso_size;
>                 skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
> -               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
> +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
>                                                          cork->gso_size);
>                 goto csum_partial;
>         }
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index aae4938f3dea..eb9a9934ac05 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1143,6 +1143,8 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
>
>                 skb_shinfo(skb)->gso_size = cork->gso_size;
>                 skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
> +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
> +                                                        cork->gso_size);
>                 goto csum_partial;
>         }
>

Fix looks good to me.

You might also want to add the original commit since you are also
addressing IPv6 changes which are unrelated to my commit that your
referenced:
Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Re: [PATCH 2/2] udp: only do GSO if # of segs > 1

[Alexander Duyck]

On Mon, Sep 30, 2019 at 3:13 PM Josh Hunt <johunt@akamai.com> wrote:
>
> Prior to this change an application sending <= 1MSS worth of data and
> enabling UDP GSO would fail if the system had SW GSO enabled, but the
> same send would succeed if HW GSO offload is enabled. In addition to this
> inconsistency the error in the SW GSO case does not get back to the
> application if sending out of a real device so the user is unaware of this
> failure.
>
> With this change we only perform GSO if the # of segments is > 1 even
> if the application has enabled segmentation. I've also updated the
> relevant udpgso selftests.
>
> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
> Signed-off-by: Josh Hunt <johunt@akamai.com>
> ---
>  net/ipv4/udp.c                       |  5 +++--
>  net/ipv6/udp.c                       |  5 +++--
>  tools/testing/selftests/net/udpgso.c | 16 ++++------------
>  3 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index be98d0b8f014..ac0baf947560 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -821,6 +821,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>         int is_udplite = IS_UDPLITE(sk);
>         int offset = skb_transport_offset(skb);
>         int len = skb->len - offset;
> +       int datalen = len - sizeof(*uh);
>         __wsum csum = 0;
>
>         /*
> @@ -832,7 +833,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>         uh->len = htons(len);
>         uh->check = 0;
>
> -       if (cork->gso_size) {
> +       if (cork->gso_size && datalen > cork->gso_size) {
>                 const int hlen = skb_network_header_len(skb) +
>                                  sizeof(struct udphdr);
>

So what about the datalen == cork->gso_size case? That would only
generate one segment wouldn't it?

Shouldn't the test really be "datalen < cork->gso_size"? That should
be the only check you need since if gso_size is 0 this statement would
always fail anyway.

Thanks.

- Alex

Re: [PATCH 1/2] udp: fix gso_segs calculations

[Willem de Bruijn]

On Mon, Sep 30, 2019 at 7:51 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Mon, Sep 30, 2019 at 3:15 PM Josh Hunt <johunt@akamai.com> wrote:
> >
> > Commit dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
> > added gso_segs calculation, but incorrectly got sizeof() the pointer and
> > not the underlying data type. It also does not account for v6 UDP GSO segs.
> >
> > Fixes: dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
> > Signed-off-by: Josh Hunt <johunt@akamai.com>
> > ---
> >  net/ipv4/udp.c | 2 +-
> >  net/ipv6/udp.c | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index cf755156a684..be98d0b8f014 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -856,7 +856,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
> >
> >                 skb_shinfo(skb)->gso_size = cork->gso_size;
> >                 skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
> > -               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
> > +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
> >                                                          cork->gso_size);
> >                 goto csum_partial;
> >         }
> > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> > index aae4938f3dea..eb9a9934ac05 100644
> > --- a/net/ipv6/udp.c
> > +++ b/net/ipv6/udp.c
> > @@ -1143,6 +1143,8 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
> >
> >                 skb_shinfo(skb)->gso_size = cork->gso_size;
> >                 skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
> > +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
> > +                                                        cork->gso_size);
> >                 goto csum_partial;
> >         }
> >
>
> Fix looks good to me.
>
> You might also want to add the original commit since you are also
> addressing IPv6 changes which are unrelated to my commit that your
> referenced:
> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
>
> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Acked-by: Willem de Bruijn <willemb@google.com>

If resending, please target net: [PATCH net v2]

Re: [PATCH 2/2] udp: only do GSO if # of segs > 1

[Willem de Bruijn]

On Mon, Sep 30, 2019 at 7:57 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Mon, Sep 30, 2019 at 3:13 PM Josh Hunt <johunt@akamai.com> wrote:
> >
> > Prior to this change an application sending <= 1MSS worth of data and
> > enabling UDP GSO would fail if the system had SW GSO enabled, but the
> > same send would succeed if HW GSO offload is enabled. In addition to this
> > inconsistency the error in the SW GSO case does not get back to the
> > application if sending out of a real device so the user is unaware of this
> > failure.
> >
> > With this change we only perform GSO if the # of segments is > 1 even
> > if the application has enabled segmentation. I've also updated the
> > relevant udpgso selftests.
> >
> > Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
> > Signed-off-by: Josh Hunt <johunt@akamai.com>
> > ---
> >  net/ipv4/udp.c                       |  5 +++--
> >  net/ipv6/udp.c                       |  5 +++--
> >  tools/testing/selftests/net/udpgso.c | 16 ++++------------
> >  3 files changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index be98d0b8f014..ac0baf947560 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -821,6 +821,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
> >         int is_udplite = IS_UDPLITE(sk);
> >         int offset = skb_transport_offset(skb);
> >         int len = skb->len - offset;
> > +       int datalen = len - sizeof(*uh);
> >         __wsum csum = 0;
> >
> >         /*
> > @@ -832,7 +833,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
> >         uh->len = htons(len);
> >         uh->check = 0;
> >
> > -       if (cork->gso_size) {
> > +       if (cork->gso_size && datalen > cork->gso_size) {
> >                 const int hlen = skb_network_header_len(skb) +
> >                                  sizeof(struct udphdr);
> >
>
> So what about the datalen == cork->gso_size case? That would only
> generate one segment wouldn't it?

Segmentation drops packets in this boundary case (not sure why).

> Shouldn't the test really be "datalen < cork->gso_size"? That should
> be the only check you need since if gso_size is 0 this statement would
> always fail anyway.
>
> Thanks.
>
> - Alex

The original choice was made to match GSO behavior of other protocols.
The drop occurs in protocol-independent skb_segment.

But I had not anticipated HW GSO to behave differently. With that,
aligning the two makes sense. Especially as UDP GSO is exposed to
userspace. Having to explicitly code a branch whether or not to pass
UDP_SEGMENT on each send based on size is confusing.

gso_size is supplied by the user. That value need not be smaller than
or equal to MTU minus headers. Some of the tests inside the branch,
especially

      if (hlen + cork->gso_size > cork->fragsize) {
              kfree_skb(skb);
              return -EINVAL;
      }

still need to be checked.

Re: [PATCH 1/2] udp: fix gso_segs calculations

[Josh Hunt]

On 9/30/19 4:51 PM, Alexander Duyck wrote:
> On Mon, Sep 30, 2019 at 3:15 PM Josh Hunt <johunt@akamai.com> wrote:
>>
>> Commit dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
>> added gso_segs calculation, but incorrectly got sizeof() the pointer and
>> not the underlying data type. It also does not account for v6 UDP GSO segs.
>>
>> Fixes: dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>> ---
>>   net/ipv4/udp.c | 2 +-
>>   net/ipv6/udp.c | 2 ++
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index cf755156a684..be98d0b8f014 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -856,7 +856,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>>
>>                  skb_shinfo(skb)->gso_size = cork->gso_size;
>>                  skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>> -               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
>> +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
>>                                                           cork->gso_size);
>>                  goto csum_partial;
>>          }
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index aae4938f3dea..eb9a9934ac05 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -1143,6 +1143,8 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
>>
>>                  skb_shinfo(skb)->gso_size = cork->gso_size;
>>                  skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>> +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
>> +                                                        cork->gso_size);
>>                  goto csum_partial;
>>          }
>>
> 
> Fix looks good to me.
> 
> You might also want to add the original commit since you are also
> addressing IPv6 changes which are unrelated to my commit that your
> referenced:
> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
> 
> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 

Thanks for the review Alex. I will make those changes and spin a v2.

Thanks!
Josh

Re: [PATCH 1/2] udp: fix gso_segs calculations

[Josh Hunt]

On 10/1/19 5:12 AM, Willem de Bruijn wrote:
> On Mon, Sep 30, 2019 at 7:51 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>>
>> On Mon, Sep 30, 2019 at 3:15 PM Josh Hunt <johunt@akamai.com> wrote:
>>>
>>> Commit dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
>>> added gso_segs calculation, but incorrectly got sizeof() the pointer and
>>> not the underlying data type. It also does not account for v6 UDP GSO segs.
>>>
>>> Fixes: dfec0ee22c0a ("udp: Record gso_segs when supporting UDP segmentation offload")
>>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>>> ---
>>>   net/ipv4/udp.c | 2 +-
>>>   net/ipv6/udp.c | 2 ++
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>>> index cf755156a684..be98d0b8f014 100644
>>> --- a/net/ipv4/udp.c
>>> +++ b/net/ipv4/udp.c
>>> @@ -856,7 +856,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>>>
>>>                  skb_shinfo(skb)->gso_size = cork->gso_size;
>>>                  skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>>> -               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
>>> +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
>>>                                                           cork->gso_size);
>>>                  goto csum_partial;
>>>          }
>>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>>> index aae4938f3dea..eb9a9934ac05 100644
>>> --- a/net/ipv6/udp.c
>>> +++ b/net/ipv6/udp.c
>>> @@ -1143,6 +1143,8 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
>>>
>>>                  skb_shinfo(skb)->gso_size = cork->gso_size;
>>>                  skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
>>> +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(*uh),
>>> +                                                        cork->gso_size);
>>>                  goto csum_partial;
>>>          }
>>>
>>
>> Fix looks good to me.
>>
>> You might also want to add the original commit since you are also
>> addressing IPv6 changes which are unrelated to my commit that your
>> referenced:
>> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
>>
>> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Acked-by: Willem de Bruijn <willemb@google.com>
> 
> If resending, please target net: [PATCH net v2]
> 

Thanks Willem. Yeah I should have targeted net originally. Will spin a v2.

Thanks!
Josh

Re: [PATCH 2/2] udp: only do GSO if # of segs > 1

[Josh Hunt]

On 9/30/19 4:56 PM, Alexander Duyck wrote:
> On Mon, Sep 30, 2019 at 3:13 PM Josh Hunt <johunt@akamai.com> wrote:
>>
>> Prior to this change an application sending <= 1MSS worth of data and
>> enabling UDP GSO would fail if the system had SW GSO enabled, but the
>> same send would succeed if HW GSO offload is enabled. In addition to this
>> inconsistency the error in the SW GSO case does not get back to the
>> application if sending out of a real device so the user is unaware of this
>> failure.
>>
>> With this change we only perform GSO if the # of segments is > 1 even
>> if the application has enabled segmentation. I've also updated the
>> relevant udpgso selftests.
>>
>> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>> ---
>>   net/ipv4/udp.c                       |  5 +++--
>>   net/ipv6/udp.c                       |  5 +++--
>>   tools/testing/selftests/net/udpgso.c | 16 ++++------------
>>   3 files changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index be98d0b8f014..ac0baf947560 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -821,6 +821,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>>          int is_udplite = IS_UDPLITE(sk);
>>          int offset = skb_transport_offset(skb);
>>          int len = skb->len - offset;
>> +       int datalen = len - sizeof(*uh);
>>          __wsum csum = 0;
>>
>>          /*
>> @@ -832,7 +833,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>>          uh->len = htons(len);
>>          uh->check = 0;
>>
>> -       if (cork->gso_size) {
>> +       if (cork->gso_size && datalen > cork->gso_size) {
>>                  const int hlen = skb_network_header_len(skb) +
>>                                   sizeof(struct udphdr);
>>
> 
> So what about the datalen == cork->gso_size case? That would only
> generate one segment wouldn't it?
> 
> Shouldn't the test really be "datalen < cork->gso_size"? That should
> be the only check you need since if gso_size is 0 this statement would
> always fail anyway.
> 
> Thanks.
> 
> - Alex
> 

Alex thanks for the review. The intent of the patch is to only use GSO 
when the # of segs > 1. The two cases you've mentioned are when the # of 
segs == 1. In those cases we don't want to set gso_size and treat this 
as a non-GSO case, skipping the if block. Let me know if I misunderstood 
your points or you'd like further clarification.

Thanks!
Josh

Re: [PATCH 2/2] udp: only do GSO if # of segs > 1

[Alexander Duyck]

On Tue, Oct 1, 2019 at 7:36 AM Josh Hunt <johunt@akamai.com> wrote:
>
>
>
> On 9/30/19 4:56 PM, Alexander Duyck wrote:
> > On Mon, Sep 30, 2019 at 3:13 PM Josh Hunt <johunt@akamai.com> wrote:
> >>
> >> Prior to this change an application sending <= 1MSS worth of data and
> >> enabling UDP GSO would fail if the system had SW GSO enabled, but the
> >> same send would succeed if HW GSO offload is enabled. In addition to this
> >> inconsistency the error in the SW GSO case does not get back to the
> >> application if sending out of a real device so the user is unaware of this
> >> failure.
> >>
> >> With this change we only perform GSO if the # of segments is > 1 even
> >> if the application has enabled segmentation. I've also updated the
> >> relevant udpgso selftests.
> >>
> >> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
> >> Signed-off-by: Josh Hunt <johunt@akamai.com>
> >> ---
> >>   net/ipv4/udp.c                       |  5 +++--
> >>   net/ipv6/udp.c                       |  5 +++--
> >>   tools/testing/selftests/net/udpgso.c | 16 ++++------------
> >>   3 files changed, 10 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> >> index be98d0b8f014..ac0baf947560 100644
> >> --- a/net/ipv4/udp.c
> >> +++ b/net/ipv4/udp.c
> >> @@ -821,6 +821,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
> >>          int is_udplite = IS_UDPLITE(sk);
> >>          int offset = skb_transport_offset(skb);
> >>          int len = skb->len - offset;
> >> +       int datalen = len - sizeof(*uh);
> >>          __wsum csum = 0;
> >>
> >>          /*
> >> @@ -832,7 +833,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
> >>          uh->len = htons(len);
> >>          uh->check = 0;
> >>
> >> -       if (cork->gso_size) {
> >> +       if (cork->gso_size && datalen > cork->gso_size) {
> >>                  const int hlen = skb_network_header_len(skb) +
> >>                                   sizeof(struct udphdr);
> >>
> >
> > So what about the datalen == cork->gso_size case? That would only
> > generate one segment wouldn't it?
> >
> > Shouldn't the test really be "datalen < cork->gso_size"? That should
> > be the only check you need since if gso_size is 0 this statement would
> > always fail anyway.
> >
> > Thanks.
> >
> > - Alex
> >
>
> Alex thanks for the review. The intent of the patch is to only use GSO
> when the # of segs > 1. The two cases you've mentioned are when the # of
> segs == 1. In those cases we don't want to set gso_size and treat this
> as a non-GSO case, skipping the if block. Let me know if I misunderstood
> your points or you'd like further clarification.
>
> Thanks!
> Josh

Your right. Somehow I got the logic backwards in my head.

Thanks.

- Alex

Re: [PATCH 2/2] udp: only do GSO if # of segs > 1

[Josh Hunt]

On 10/1/19 5:22 AM, Willem de Bruijn wrote:
> On Mon, Sep 30, 2019 at 7:57 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>>
>> On Mon, Sep 30, 2019 at 3:13 PM Josh Hunt <johunt@akamai.com> wrote:
>>>
>>> Prior to this change an application sending <= 1MSS worth of data and
>>> enabling UDP GSO would fail if the system had SW GSO enabled, but the
>>> same send would succeed if HW GSO offload is enabled. In addition to this
>>> inconsistency the error in the SW GSO case does not get back to the
>>> application if sending out of a real device so the user is unaware of this
>>> failure.
>>>
>>> With this change we only perform GSO if the # of segments is > 1 even
>>> if the application has enabled segmentation. I've also updated the
>>> relevant udpgso selftests.
>>>
>>> Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
>>> Signed-off-by: Josh Hunt <johunt@akamai.com>
>>> ---
>>>   net/ipv4/udp.c                       |  5 +++--
>>>   net/ipv6/udp.c                       |  5 +++--
>>>   tools/testing/selftests/net/udpgso.c | 16 ++++------------
>>>   3 files changed, 10 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>>> index be98d0b8f014..ac0baf947560 100644
>>> --- a/net/ipv4/udp.c
>>> +++ b/net/ipv4/udp.c
>>> @@ -821,6 +821,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>>>          int is_udplite = IS_UDPLITE(sk);
>>>          int offset = skb_transport_offset(skb);
>>>          int len = skb->len - offset;
>>> +       int datalen = len - sizeof(*uh);
>>>          __wsum csum = 0;
>>>
>>>          /*
>>> @@ -832,7 +833,7 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>>>          uh->len = htons(len);
>>>          uh->check = 0;
>>>
>>> -       if (cork->gso_size) {
>>> +       if (cork->gso_size && datalen > cork->gso_size) {
>>>                  const int hlen = skb_network_header_len(skb) +
>>>                                   sizeof(struct udphdr);
>>>
>>
>> So what about the datalen == cork->gso_size case? That would only
>> generate one segment wouldn't it?
> 
> Segmentation drops packets in this boundary case (not sure why).
> 
>> Shouldn't the test really be "datalen < cork->gso_size"? That should
>> be the only check you need since if gso_size is 0 this statement would
>> always fail anyway.
>>
>> Thanks.
>>
>> - Alex
> 
> The original choice was made to match GSO behavior of other protocols.
> The drop occurs in protocol-independent skb_segment.
> 
> But I had not anticipated HW GSO to behave differently. With that,

Right and for HW GSO we don't call skb_segment().

> aligning the two makes sense. Especially as UDP GSO is exposed to
> userspace. Having to explicitly code a branch whether or not to pass
> UDP_SEGMENT on each send based on size is confusing.
> 
> gso_size is supplied by the user. That value need not be smaller than
> or equal to MTU minus headers. Some of the tests inside the branch,
> especially
> 
>        if (hlen + cork->gso_size > cork->fragsize) {
>                kfree_skb(skb);
>                return -EINVAL;
>        }
> 
> still need to be checked.
> 

Thanks for the review Willem. I will look at those checks and send a v2.

Thanks!
Josh