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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iKEP+LZGHh=ud_c0_RCNM2OHRxF0jeg1EkPpNmTnaXLSQ@mail.gmail.com>
Date:   Tue, 3 Jan 2023 17:10:24 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Guillaume Nault <gnault@...hat.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 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()


>
> > (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.
> >
>