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: Windows password security audit tool. GUI, reports in PDF.

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

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

On Thu, Aug 25, 2016 at 09:02:33AM -0700, Alexander Duyck wrote:
> On Thu, Aug 25, 2016 at 4:00 AM, Steffen Klassert
> > +
> > +                       skb_walk_frags(head_skb, segs) {
> > +                               len -= segs->len;
> > +                       }
> 
> One thought I had is that you may want to add a check here to make
> certain that the skb frags have a headlen of 0.  If they don't you
> would probably need to bail.

Why do you think we need this check? Is it because the assumption that
all excluding the last skb have the same size might be wrong in this
case?

> >
> > +       gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL);
> > +
> >         outer_hlen = skb_tnl_header_len(skb);
> >         udp_offset = outer_hlen - tnl_hlen;
> >         skb = segs;
> > @@ -117,7 +119,7 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
> >                  * will be using a length value equal to only one MSS sized
> >                  * segment instead of the entire frame.
> >                  */
> > -               if (skb_is_gso(skb)) {
> > +               if (skb_is_gso(skb) && gso_partial) {
> >                         uh->len = htons(skb_shinfo(skb)->gso_size +
> >                                         SKB_GSO_CB(skb)->data_offset +
> >                                         skb->head - (unsigned char *)uh);
> 
> It might be worth while to reverse these checks.  You can probably
> save a few cycles if you check for gso_partial first and then
> skb_is_gso instead of the current ordering.

Maybe we can even remove the skb_is_gso check, the gso_partial
check should be sufficient.