netdev - Re: [PATCH net v4 2/3] Treewide: Stop corrupting socket's task

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] [day] [month] [year] [list]
Message-ID: <Y7SuNg4Ewesb7l9T@debian>
Date:   Tue, 3 Jan 2023 23:37:42 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Benjamin Coddington <bcodding@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Philipp Reisner <philipp.reisner@...bit.com>,
        Lars Ellenberg <lars.ellenberg@...bit.com>,
        Christoph Böhmwalder 
        <christoph.boehmwalder@...bit.com>, Jens Axboe <axboe@...nel.dk>,
        Josef Bacik <josef@...icpanda.com>,
        Keith Busch <kbusch@...nel.org>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        Lee Duncan <lduncan@...e.com>, Chris Leech <cleech@...hat.com>,
        Mike Christie <michael.christie@...cle.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Valentina Manea <valentina.manea.m@...il.com>,
        Shuah Khan <shuah@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        David Howells <dhowells@...hat.com>,
        Marc Dionne <marc.dionne@...istor.com>,
        Steve French <sfrench@...ba.org>,
        Christine Caulfield <ccaulfie@...hat.com>,
        David Teigland <teigland@...hat.com>,
        Mark Fasheh <mark@...heh.com>,
        Joel Becker <jlbec@...lplan.org>,
        Joseph Qi <joseph.qi@...ux.alibaba.com>,
        Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        Dominique Martinet <asmadeus@...ewreck.org>,
        Ilya Dryomov <idryomov@...il.com>,
        Xiubo Li <xiubli@...hat.com>,
        Chuck Lever <chuck.lever@...cle.com>,
        Jeff Layton <jlayton@...nel.org>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna@...nel.org>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net v4 2/3] Treewide: Stop corrupting socket's task_frag

On Tue, Jan 03, 2023 at 05:10:24PM +0100, Eric Dumazet wrote:
> On Tue, Jan 3, 2023 at 4:14 PM Guillaume Nault <gnault@...hat.com> wrote:
> >
> > On Tue, Jan 03, 2023 at 03:26:27PM +0100, Eric Dumazet wrote:
> > > On Fri, Dec 16, 2022 at 1:45 PM Benjamin Coddington <bcodding@...hat.com> wrote:
> > > >
> > > > Since moving to memalloc_nofs_save/restore, SUNRPC has stopped setting the
> > > > GFP_NOIO flag on sk_allocation which the networking system uses to decide
> > > > when it is safe to use current->task_frag.  The results of this are
> > > > unexpected corruption in task_frag when SUNRPC is involved in memory
> > > > reclaim.
> > > >
> > > > The corruption can be seen in crashes, but the root cause is often
> > > > difficult to ascertain as a crashing machine's stack trace will have no
> > > > evidence of being near NFS or SUNRPC code.  I believe this problem to
> > > > be much more pervasive than reports to the community may indicate.
> > > >
> > > > Fix this by having kernel users of sockets that may corrupt task_frag due
> > > > to reclaim set sk_use_task_frag = false.  Preemptively correcting this
> > > > situation for users that still set sk_allocation allows them to convert to
> > > > memalloc_nofs_save/restore without the same unexpected corruptions that are
> > > > sure to follow, unlikely to show up in testing, and difficult to bisect.
> > > >
> > >
> > > I am back from PTO.
> > >
> > > It seems inet_ctl_sock_create() has been forgotten.
> > >
> > > Without following fix, ICMP messages sent from softirq would corrupt
> > > innocent thread task_frag.
> >
> > I didn't consider setting ->sk_use_task_frag on ICMP sockets as my
> > understanding was that only TCP and ip_append_data() could eventually
> > call sk_page_frag(). Therefore, I didn't see how ICMP sockets could be
> > affected. Did I miss something?
> 
> net/ipv4/ping.c
> 
> ICMP uses per-cpu sockets (look in icmp_init(), icmp_xmit_lock()...)
> 
> icmp_rcv()
>   -> icmp_echo()
>      -> icmp_reply()
>        -> icmp_push_reply()
>          -> ip_append_data()
>            -> sk_page_frag_refill()

Thanks, that looks so obvious now :/
Sorry for missing that case.

> >
> > > (I will submit this patch formally a bit later today)
> > >
> > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> > > index ab4a06be489b5d410cec603bf56248d31dbc90dd..6c0ec27899431eb56e2f9d0c3a936b77f44ccaca
> > > 100644
> > > --- a/net/ipv4/af_inet.c
> > > +++ b/net/ipv4/af_inet.c
> > > @@ -1665,6 +1665,7 @@ int inet_ctl_sock_create(struct sock **sk,
> > > unsigned short family,
> > >         if (rc == 0) {
> > >                 *sk = sock->sk;
> > >                 (*sk)->sk_allocation = GFP_ATOMIC;
> > > +               (*sk)->sk_use_task_frag = false;
> > >                 /*
> > >                  * Unhash it so that IP input processing does not even see it,
> > >                  * we do not wish this socket to see incoming packets.
> > >
> >
>