Re: [PATCH 1/2] nfs: Fix spurios EPERM when mkdir of existing dentry

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On Jul 7, 2016, at 12:52, Oleg Drokin <green@xxxxxxxxxxxxxx> wrote:
> 
> 
> On Jul 7, 2016, at 12:16 PM, Trond Myklebust wrote:
> 
>> 
>>> On Jul 7, 2016, at 01:53, Oleg Drokin <green@xxxxxxxxxxxxxx> wrote:
>>> 
>>> It's great when we can shave an extra RPC, but not at the expense
>>> of correctness.
>>> We should not return EPERM (from vfs_create/mknod/mkdir) if the
>>> name already exists, even if we have no write access in parent.
>>> 
>>> Since the check in nfs_permission is clearly not enough to stave
>>> off this, just throw in the extra READ access to actually
>>> go through.
>>> 
>>> Signed-off-by: Oleg Drokin <green@xxxxxxxxxxxxxx>
>>> ---
>>> fs/nfs/dir.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>>> index d8015a0..8c7835b 100644
>>> --- a/fs/nfs/dir.c
>>> +++ b/fs/nfs/dir.c
>>> @@ -1383,8 +1383,10 @@ struct dentry *nfs_lookup(struct inode *dir, struct dentry * dentry, unsigned in
>>> 	/*
>>> 	 * If we're doing an exclusive create, optimize away the lookup
>>> 	 * but don't hash the dentry.
>>> +	 * This optimization only works if we can write in the parent.
>>> 	 */
>>> -	if (nfs_is_exclusive_create(dir, flags))
>>> +	if (nfs_is_exclusive_create(dir, flags) &&
>>> +	    (inode_permission(dir, MAY_WRITE | MAY_READ | MAY_EXEC) == 0))
>>> 		return NULL;
>>> 
>> 
>> NACK. The only write permission we should care about on the client side is whether or not the filesystem is mounted read-only. All other permissions are checked by the server.
> 
> Right. This was mostly a discussion piece.
> The problem here is nfs_permission() returns 0 if you check for
> inode_permission(dir, MAY_WRITE | MAY_EXEC) (as in may_create), but then
> some other checks in the kernel still catch on to the fact that the directory
> is not writeable, so we have a premature failure with EPERM and server never sees
> this request which breaks things.

Are these VFS level checks? Which ones?

> 
> (the read-only mount is not handled as well at the moment of course and my patch
> does not address this issue either, but it's easier to address in the VFS, like
> in filename_create() or something).
> 
> I see that two major consumers of this nfs_permission MAY_WRITE|!MAY_READ check
> are creates and deletes and with deletes we had a lookup already, so it already
> looked up the child and revalidated the parent.
> For creates, a revalidation still might be needed, I guess and that was the main driver
> behind this check? And that only when you do current dir creates, because otherwise
> the parent would have been revalidated in lookup?
> Is this the major case why that check is actually there?
> 
> Just trying to see how to approach this better without breaking the applications.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux