Return-Path: Received: from us-smtp-delivery-194.mimecast.com ([216.205.24.194]:24888 "EHLO us-smtp-delivery-194.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751220AbcGGQQe convert rfc822-to-8bit (ORCPT ); Thu, 7 Jul 2016 12:16:34 -0400 From: Trond Myklebust To: Oleg Drokin CC: Jeff Layton , Viro Alexander , "linux-fsdevel@vger.kernel.org" , "linux-nfs@vger.kernel.org" Subject: Re: [PATCH 1/2] nfs: Fix spurios EPERM when mkdir of existing dentry Date: Thu, 7 Jul 2016 16:16:12 +0000 Message-ID: <9F6E5530-CBA4-4A0A-BAE1-72AAC28DB4D1@primarydata.com> References: <1467870827-2959489-1-git-send-email-green@linuxhacker.ru> <1467870827-2959489-2-git-send-email-green@linuxhacker.ru> In-Reply-To: <1467870827-2959489-2-git-send-email-green@linuxhacker.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252 Sender: linux-nfs-owner@vger.kernel.org List-ID: > On Jul 7, 2016, at 01:53, Oleg Drokin 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 > --- > 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. Cheers Trond