On 10/1/19 5:22 AM, Willem de Bruijn wrote:

On Mon, Sep 30, 2019 at 7:57 PM Alexander Duyck
[off-list ref] wrote:

quoted

On Mon, Sep 30, 2019 at 3:13 PM Josh Hunt [off-list ref] wrote:

quoted

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 <redacted>
---
  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).

quoted

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