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: <Y7RGSbWX0L4EoA8W@debian>
Date:   Tue, 3 Jan 2023 16:14:17 +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 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?

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