netdev - Re: [PATCH net 1/4] net: fix double-free on fraglist GSO skbs

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: <3e41198f79b4d63812e3862ca688507bf3f7d65d.camel@redhat.com>
Date:   Mon, 10 May 2021 17:37:58 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net 1/4] net: fix double-free on fraglist GSO skbs

On Fri, 2021-05-07 at 10:46 +0200, Paolo Abeni wrote:
> On Thu, 2021-05-06 at 14:17 -0700, Jakub Kicinski wrote:
> > On Thu, 06 May 2021 17:55:36 +0200 Paolo Abeni wrote:
> > > On Thu, 2021-05-06 at 10:32 -0400, Willem de Bruijn wrote:
> > > > On Thu, May 6, 2021 at 7:07 AM Paolo Abeni <pabeni@...hat.com> wrote:  
> > > > > If we want to be safe about future possible sock_wfree users, I think
> > > > > the approach here should be different: in skb_segment(), tail-  
> > > > > > destructor is expected to be NULL, while skb_segment_list(), all the  
> > > > > list skbs can be owned by the same socket. Possibly we could open-
> > > > > code skb_release_head_state(), omitting the skb orphaning part
> > > > > for sock_wfree() destructor.
> > > > > 
> > > > > Note that the this is not currently needed - sock_wfree destructor
> > > > > can't reach there.
> > > > > 
> > > > > Given all the above, I'm unsure if you are fine with (or at least do
> > > > > not oppose to) the code proposed in this patch?  
> > > > 
> > > > Yes. Thanks for clarifying, Paolo.  
> > > 
> > > Thank you for reviewing!
> > > 
> > > @David, @Jakub: I see this series is already archived as "change
> > > requested", should I repost?
> > 
> > Yes, please. Patch 2 adds two new sparse warnings. 
> > 
> > I think you need csum_unfold() to go from __sum16 to __wsum.
> 
> Yes, indeed. I'll send a v2 with such change, thanks!

It's taking [much] more than expected, as it turned out that thare are
still a number of case where the tx csum is uncorrect.

If the traffic comes from a veth we don't have a valid th->csum value
at GRO time, setting ip_summed to CHECKSUM_UNNECESSARY - as the current
code does - looks wrong.
@Steffen: I see in the original discussion about GRO_FRAGLIST
introduction that you wanted the GRO packets to be CHECKSUM_UNNECESSARY
to avoid csum modification in fwd path. I guess that choice was mostily
due performance reasons, to avoid touching the aggregated pkts header
at gso_segment_list time, but it looks like it's quite bug prone. If
so, I'm unsure the performance gain is worty. I propose to switch to
CHECKSUM_PARTIAL. Would you be ok with that?

Thanks,

Paolo