Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Apr 2023 09:32:43 +1000
From:   Dave Chinner <david@fromorbit.com>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Jeff Layton <jlayton@kernel.org>,
        Frank van der Linden <fllinden@amazon.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH] nfsd: don't use GFP_KERNEL from
 nfsd_getxattr()/nfsd_listxattr()
Message-ID: <20230419233243.GM447837@dread.disaster.area>
References: <72bf692e-bb6b-c1f2-d1ba-3205ab649b43@I-love.SAKURA.ne.jp>
 <4BC7955B-40E4-4A43-B2D1-2E9302E84337@oracle.com>
 <b014047a-4a70-b38f-c5bb-01bc3c53d6f2@I-love.SAKURA.ne.jp>
 <aee35d52ab19e7e95f69742be8329764db72cbf8.camel@kernel.org>
 <c310695e-4279-b1a7-5c2a-2771cc19aa66@I-love.SAKURA.ne.jp>
 <7246a80ae33244a4553bbc0ca9e771ce8143d97b.camel@kernel.org>
 <20230416233758.GD447837@dread.disaster.area>
 <A23409BB-9BA1-44E5-96A8-C080B417CCB5@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <A23409BB-9BA1-44E5-96A8-C080B417CCB5@oracle.com>
Precedence: bulk

On Wed, Apr 19, 2023 at 01:51:12PM +0000, Chuck Lever III wrote:
> 
> 
> > On Apr 16, 2023, at 7:37 PM, Dave Chinner <david@fromorbit.com> wrote:
> > 
> > On Sun, Apr 16, 2023 at 07:51:41AM -0400, Jeff Layton wrote:
> >> On Sun, 2023-04-16 at 08:21 +0900, Tetsuo Handa wrote:
> >>> On 2023/04/16 3:40, Jeff Layton wrote:
> >>>> On Sun, 2023-04-16 at 02:11 +0900, Tetsuo Handa wrote:
> >>>>> On 2023/04/16 1:13, Chuck Lever III wrote:
> >>>>>>> On Apr 15, 2023, at 7:07 AM, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> >>>>>>> 
> >>>>>>> Since GFP_KERNEL is GFP_NOFS | __GFP_FS, usage like GFP_KERNEL | GFP_NOFS
> >>>>>>> does not make sense. Drop __GFP_FS flag in order to avoid deadlock.
> >>>>>> 
> >>>>>> The server side threads run in process context. GFP_KERNEL
> >>>>>> is safe to use here -- as Jeff said, this code is not in
> >>>>>> the server's reclaim path. Plenty of other call sites in
> >>>>>> the NFS server code use GFP_KERNEL.
> >>>>> 
> >>>>> GFP_KERNEL memory allocation calls filesystem's shrinker functions
> >>>>> because of __GFP_FS flag. My understanding is
> >>>>> 
> >>>>>  Whether this code is in memory reclaim path or not is irrelevant.
> >>>>>  Whether memory reclaim path might hold lock or not is relevant.
> >>>>> 
> >>>>> . Therefore, question is, does nfsd hold i_rwsem during memory reclaim path?
> >>>>> 
> >>>> 
> >>>> No. At the time of these allocations, the i_rwsem is not held.
> >>> 
> >>> Excuse me? nfsd_getxattr()/nfsd_listxattr() _are_ holding i_rwsem
> >>> via inode_lock_shared(inode) before kvmalloc(GFP_KERNEL | GFP_NOFS) allocation.
> >>> That's why
> >>> 
> >>> /*
> >>>  * We're holding i_rwsem - use GFP_NOFS.
> >>>  */
> >>> 
> >>> is explicitly there in nfsd_listxattr() side.
> > 
> > You can do GFP_KERNEL allocations holding the i_rwsem just fine.
> > All that it requires is the caller holds a reference to the inode,
> > and at that point inode will should skip the given inode without
> > every locking it.
> 
> This suggests that the fix is to replace "GFP_KERNEL | GFP_NOFS"
> with "GFP_KERNEL" /and/ ensure those paths are holding an
> appropriate inode reference.

If the code that provided the inode to nfsd_listxattr() did not
already have an active inode reference in the first place then there
are much, much bigger UAF problems to worry about than simple
memory reclaim deadlocks.

That said, nfsd_listxattr() does:

        dentry = fhp->fh_dentry;
        inode = d_inode(dentry);
        *lenp = 0;

        inode_lock_shared(inode);

        len = vfs_listxattr(dentry, NULL, 0);

Given that a dentry pointing to an inode *must* hold an active
reference to that inode, I don't see how it is possible this code
path could be using an unreferenced inode.

nfsd_getxattr() has a similar code fragment to obtain the inode as
well, so same goes for that...

-Dave.
-- 
Dave Chinner
david@fromorbit.com