> On Apr 20, 2023, at 10:05 AM, Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> 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
