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

[Guillaume Nault <gnault@redhat.com>]

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@redhat.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@redhat.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.
> > >
> >
>