Re: [PATCH] nfsd: don't use GFP_KERNEL from nfsd_getxattr()/nfsd_listxattr()

[Chuck Lever III <chuck.lever@oracle.com>]

> On Apr 20, 2023, at 10:05 AM, Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> On 2023/04/20 22:41, Chuck Lever III wrote:
>>> 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, thanks for handling the due diligence! I was not 100% sure
>> about code that handles xattrs rather than the primary byte stream
>> of a file.
>> 
>> Tetsuo, you can send a v2, or just let me know and I will make
>> a patch to correct the GFP flags.
> 
> So, this inode_lock_shared() was there with an intention to make sure that
> xattr of inode inside the exported filesystem does not change between
> vfs_listxattr(dentry, NULL, 0) and vfs_listxattr(dentry, buf, len),
> wasn't it?
> 
> Then, we can remove this inode_lock_shared() by adding a "goto retry;"
> when vfs_listxattr(dentry, buf, len) failed with out of buffer size
> due to a race condition, can't we?
> 
> I leave replacing inode lock with retry path and removing GFP_NOFS to you.

Jeff similarly mentioned to me the tactic of simply retrying if the
retrieved listxattr contents exceed the size of the buffer. I think
I'd like to leave that for another time, as it seems to be safe to
use GFP_KERNEL with the inode lock held in this case.


--
Chuck Lever