netdev - Re: [PATCH net-next v1] gso: Support partial splitting at the frag

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 24 Aug 2016 14:25:29 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>,
        Steffen Klassert <steffen.klassert@...unet.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Alexander Duyck <alexander.h.duyck@...el.com>
Subject: Re: [PATCH net-next v1] gso: Support partial splitting at the
 frag_list pointer

Em 24-08-2016 13:27, Alexander Duyck escreveu:
> On Wed, Aug 24, 2016 at 2:32 AM, Steffen Klassert
> <steffen.klassert@...unet.com> wrote:
>> On Tue, Aug 23, 2016 at 07:47:32AM -0700, Alexander Duyck wrote:
>>> On Mon, Aug 22, 2016 at 10:20 PM, Steffen Klassert
>>> <steffen.klassert@...unet.com> wrote:
>>>> Since commit 8a29111c7 ("net: gro: allow to build full sized skb")
>>>> gro may build buffers with a frag_list. This can hurt forwarding
>>>> because most NICs can't offload such packets, they need to be
>>>> segmented in software. This patch splits buffers with a frag_list
>>>> at the frag_list pointer into buffers that can be TSO offloaded.
>>>>
>>>> Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com>
>>>> ---
>>>>  net/core/skbuff.c      | 89 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  net/ipv4/af_inet.c     |  7 ++--
>>>>  net/ipv4/gre_offload.c |  7 +++-
>>>>  net/ipv4/tcp_offload.c |  3 ++
>>>>  net/ipv4/udp_offload.c |  9 +++--
>>>>  net/ipv6/ip6_offload.c |  6 +++-
>>>>  6 files changed, 114 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>> index 3864b4b6..a614e9d 100644
>>>> --- a/net/core/skbuff.c
>>>> +++ b/net/core/skbuff.c
>>>> @@ -3078,6 +3078,92 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>>>>         sg = !!(features & NETIF_F_SG);
>>>>         csum = !!can_checksum_protocol(features, proto);
>>>>
>>>> +       headroom = skb_headroom(head_skb);
>>>> +
>>>> +       if (list_skb && net_gso_ok(features, skb_shinfo(head_skb)->gso_type) &&
>>>> +           csum && sg && (mss != GSO_BY_FRAGS) &&
>>>> +           !(features & NETIF_F_GSO_PARTIAL)) {
>>>
>>> Does this really need to be mutually exclusive with
>>> NETIF_F_GSO_PARTIAL and GSO_BY_FRAGS?
>>
>> It should be possible to extend this to NETIF_F_GSO_PARTIAL but
>> I have no test for this. Regarding GSO_BY_FRAGS, this is rather
>> new and just used for sctp. I don't know what sctp does with
>> GSO_BY_FRAGS.
>
> I'm adding Marcelo as he could probably explain the GSO_BY_FRAGS
> functionality better than I could since he is the original author.
>
> If I recall GSO_BY_FRAGS does something similar to what you are doing,
> although I believe it doesn't carry any data in the first buffer other
> than just a header.  I believe the idea behind GSO_BY_FRAGS was to
> allow for segmenting a frame at the frag_list level instead of having
> it done just based on MSS.  That was the only reason why I brought it
> up.
>

That's exactly it.

On this no data in the first buffer limitation, we probably can allow it 
have some data in there. It was done this way just because sctp is using 
skb_gro_receive() to build such skb and this was the way I found to get 
such frag_list skb generated by it, thus preserving frame boundaries.

For using GSO_BY_FRAGS in gso_size, it's how skb_is_gso() returns true, 
but it's similar to the SKB_GSO_PARTIAL rationale in here. We can make 
sctp also flag it as SKB_GSO_PARTIAL if needed I guess, in case you need 
to maintain gso_size value.

   Marcelo

> In you case though we maybe be able to make this easier.  If I am not
> mistaken I believe we should have the main skb, and any in the chain
> excluding the last containing the same amount of data.  That being the
> case we should be able to determine the size that you would need to
> segment at by taking skb->len, and removing the length of all the
> skbuffs hanging off of frag_list.  At that point you just use that as
> your MSS for segmentation and it should break things up so that you
> have a series of equal sized segments split as the frag_list buffer
> boundaries.
>
> After that all that is left is to update the gso info for the buffers.
> For GSO_PARTIAL I was handling that on the first segment only.  For
> this change you would need to update that code to address the fact
> that you would have to determine the number of segments on the first
> frame and the last since the last could be less than the first, but
> all of the others in-between should have the same number of segments.
>
> - Alex
>