netdev - Re: [PATCH net-next 02/10] udp: add gso

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF=yD-K2Oqf14oC-iH2sj+0cWdimdoAuY4HOYgF=BXVFbQMhEQ@mail.gmail.com>
Date:   Wed, 25 Apr 2018 16:51:41 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Dimitris Michailidis <dmichail@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next 02/10] udp: add gso

> It might be nice if you could break this into two patches. One for
> actually doing the GSO in software, and another enabling the stack to
> request it.

Okay.

>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index d274059529eb..a4a5c0c5cba8 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -573,6 +573,8 @@ enum {
>>         SKB_GSO_ESP = 1 << 15,
>>
>>         SKB_GSO_UDP = 1 << 16,
>> +
>> +       SKB_GSO_UDP_L4 = 1 << 17,
>>  };
>>
>
> Part of me really wishes we could just rename SKB_GSO_UDP to something
> like SKB_GFO_UDP since GSO implies "segmentation" but what we really
> mean is "fragmentation" in the case of the existing UDP offload.

I have been itching to rename the UFO flag, though I then find
SKB_GSO_UFO easier to differentiate from SKB_GSO_UDP
than SKB_GFO_UDP.

But, the uapi headers will continue to have
VIRTIO_NET_HDR_GSO_UDP for UFO, so changing this
might only further complicate things.

>> +struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>> +                                 netdev_features_t features,
>> +                                 unsigned int mss, __sum16 check)
>> +{
>> +       struct udphdr *uh = udp_hdr(gso_skb);
>> +       struct sk_buff *segs;
>> +       unsigned int hdrlen;
>> +
>> +       if (gso_skb->len <= sizeof(*uh) + mss)
>> +               return ERR_PTR(-EINVAL);
>> +
>> +       uh->len = htons(sizeof(*uh) + mss);
>> +       uh->check = check;
>> +       skb_pull(gso_skb, sizeof(*uh));
>> +       hdrlen = gso_skb->data - skb_mac_header(gso_skb);
>
> Normally I don't believe we modify the headers before calling
> skb_segment. Normally that happens after via a while (skb->next) {}
> type loop.

Setting the header for all but the last segment before segmentation
avoided the need for a while loop, at least before the wmem_alloc
patch.

With that patch, the argument no longer really holds, so I can convert
if that is needed for other features, like GSO_PARTIAL.

> That way for things like GSO_PARTIAL we can update after segmentation
> since there are only going to be 2 segments most likely instead of
> multiple MSS sized segments.

I don't quite follow. Which two segments?

>> +
>> +       segs = skb_segment(gso_skb, features);
>> +       if (unlikely(IS_ERR_OR_NULL(segs)))
>> +               return segs;
>> +
>> +       /* If last packet is not full, fix up its header */
>> +       if (segs->prev->len != hdrlen + mss) {
>> +               unsigned int mss_last = segs->prev->len - hdrlen;
>> +
>> +               uh = udp_hdr(segs->prev);
>> +               uh->len = htons(sizeof(*uh) + mss_last);
>> +               csum_replace2(&uh->check, htons(mss), htons(mss_last));
>> +       }
>> +
>
> You could probably just assume that this last segment is always going
> to need to be updated regardless. If you are doing any sort of
> segmentation the last segment will always need the length and checksum
> updated anyway, and since you probably need to move over to the "while
> (skb->next)" style loop then you could just assume the last segment
> needs updating.

Ack. If we have to do a loop and set everything in there, I'll just write
all headers unconditionally.