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

[Guillaume Nault <gnault@redhat.com>]

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?

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