On Jul 7, 2016, at 1:27 PM, Trond Myklebust wrote: > >> On Jul 7, 2016, at 13:07, Oleg Drokin <green@xxxxxxxxxxxxxx> wrote: >> >> >> On Jul 7, 2016, at 12:59 PM, Trond Myklebust wrote: >> >>> >>>> 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? >> >> Yes, VFS level I believe. >> For Lustre it's may_create() from vfs_mkdir() that stops us short >> and the Lustre patch is effective. >> but for NFS this must be something else and I did not trace >> it completely. One of the security checks, I guess? >> if NFS patch is changed to check inode_permission(dir, MAY_OPEN | MAY_EXEC) >> as in Lustre, that returns 0 no matter what. >> >> This is trivial to reproduce too. > > So, should we be ignoring the MAY_EXEC flag when the VFS asks for inode_permission(dir, MAY_WRITE|MAY_EXEC)? I suspect that is the problem here. I am not sure what do you mean here. When vfs asks for inode_permission(dir, MAY_WRITE|MAY_EXEC) NFS already returns 0 (i.e. everything is ok, go ahead). But it's all futile because some other check somewhere still catches on to the fact that the directory is not writeable and does not let the mkdir to actually happen in the FS itself. >>>> (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-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
