| Message ID | 1451046656-26319-1-git-send-email-buczek@molgen.mpg.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 25, 2015 at 7:30 AM, Donald Buczek <buczek@molgen.mpg.de> wrote: > This patch fixes a problem, that a nfs4 client incorrectly denies > execute access based on outdated file mode (missing 'x' bit). > After the mode on the server is 'fixed' (chmod +x) further execution > attempts continue to fail, because the nfs ACCESS call updates > the access parameter but not the mode parameter or the mode in > the inode. > > The access check based on the file mode is not required, because > the server already verified the clients rights. > > Remove the test. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109771 > Signed-off-by: Donald Buczek <buczek@molgen.mpg.de> > --- > fs/nfs/dir.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index ce5a218..ffc25b0 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -2481,9 +2481,6 @@ force_lookup: > res = PTR_ERR(cred); > } > out: > - if (!res && (mask & MAY_EXEC) && !execute_ok(inode)) > - res = -EACCES; > - > dfprintk(VFS, "NFS: permission(%s/%lu), mask=0x%x, res=%d\n", > inode->i_sb->s_id, inode->i_ino, mask, res); > return res; > My main question here is why the client isn't picking up the changed mode bits here? All open() calls should be asking for the full set of attributes as part of the OPEN COMPOUND rpc call. Cheers Trond -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26.12.2015 19:36, Trond Myklebust wrote: > On Fri, Dec 25, 2015 at 7:30 AM, Donald Buczek <buczek@molgen.mpg.de> wrote: >> This patch fixes a problem, that a nfs4 client incorrectly denies >> execute access based on outdated file mode (missing 'x' bit). >> After the mode on the server is 'fixed' (chmod +x) further execution >> attempts continue to fail, because the nfs ACCESS call updates >> the access parameter but not the mode parameter or the mode in >> the inode. >> >> The access check based on the file mode is not required, because >> the server already verified the clients rights. >> >> Remove the test. >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109771 >> Signed-off-by: Donald Buczek <buczek@molgen.mpg.de> >> --- >> fs/nfs/dir.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c >> index ce5a218..ffc25b0 100644 >> --- a/fs/nfs/dir.c >> +++ b/fs/nfs/dir.c >> @@ -2481,9 +2481,6 @@ force_lookup: >> res = PTR_ERR(cred); >> } >> out: >> - if (!res && (mask & MAY_EXEC) && !execute_ok(inode)) >> - res = -EACCES; >> - >> dfprintk(VFS, "NFS: permission(%s/%lu), mask=0x%x, res=%d\n", >> inode->i_sb->s_id, inode->i_ino, mask, res); >> return res; >> > My main question here is why the client isn't picking up the changed > mode bits here? All open() calls should be asking for the full set of > attributes as part of the OPEN COMPOUND rpc call. > > Cheers > Trond Its from fs/namei.c do_last() : > finish_open_created: > error = may_open(&nd->path, acc_mode, open_flag); > if (error) > goto out; > > BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */ > error = vfs_open(&nd->path, file, current_cred()); may_open() -> inode_permission() -> __inode_permission() -> do_inode_permission() -> inode->i_op->permission() -> nfs_permission() first vfs_open() -> do_dentry_open() -> inode->i_fop->open() -> nfs4_file_open() later Merry Christmas Donald
On Sat, Dec 26, 2015 at 6:58 PM, Donald Buczek <buczek@molgen.mpg.de> wrote: > On 26.12.2015 19:36, Trond Myklebust wrote: >> >> On Fri, Dec 25, 2015 at 7:30 AM, Donald Buczek <buczek@molgen.mpg.de> >> wrote: >>> >>> This patch fixes a problem, that a nfs4 client incorrectly denies >>> execute access based on outdated file mode (missing 'x' bit). >>> After the mode on the server is 'fixed' (chmod +x) further execution >>> attempts continue to fail, because the nfs ACCESS call updates >>> the access parameter but not the mode parameter or the mode in >>> the inode. >>> >>> The access check based on the file mode is not required, because >>> the server already verified the clients rights. >>> >>> Remove the test. >>> >>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109771 >>> Signed-off-by: Donald Buczek <buczek@molgen.mpg.de> >>> --- >>> fs/nfs/dir.c | 3 --- >>> 1 file changed, 3 deletions(-) >>> >>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c >>> index ce5a218..ffc25b0 100644 >>> --- a/fs/nfs/dir.c >>> +++ b/fs/nfs/dir.c >>> @@ -2481,9 +2481,6 @@ force_lookup: >>> res = PTR_ERR(cred); >>> } >>> out: >>> - if (!res && (mask & MAY_EXEC) && !execute_ok(inode)) >>> - res = -EACCES; >>> - >>> dfprintk(VFS, "NFS: permission(%s/%lu), mask=0x%x, res=%d\n", >>> inode->i_sb->s_id, inode->i_ino, mask, res); >>> return res; >>> >> My main question here is why the client isn't picking up the changed >> mode bits here? All open() calls should be asking for the full set of >> attributes as part of the OPEN COMPOUND rpc call. >> >> Cheers >> Trond > > > Its from fs/namei.c do_last() : > >> finish_open_created: >> error = may_open(&nd->path, acc_mode, open_flag); >> if (error) >> goto out; >> >> BUG_ON(*opened & FILE_OPENED); /* once it's opened, it's opened */ >> error = vfs_open(&nd->path, file, current_cred()); > > > > may_open() -> inode_permission() -> __inode_permission() -> > do_inode_permission() -> inode->i_op->permission() -> nfs_permission() > first > > vfs_open() -> do_dentry_open() -> inode->i_fop->open() -> nfs4_file_open() > later Ah... IOW: That so called "fast path" crap in do_last() is screwing us over by forcing us to to 2 ACCESS calls. The first being done for no good reason by the VFS, and the second being done in the OPEN call to the server in the NFS client. > Merry Christmas Suggestions Al? Trond -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 26, 2015 at 07:11:16PM -0500, Trond Myklebust wrote: > Ah... IOW: That so called "fast path" crap in do_last() is screwing us > over by forcing us to to 2 ACCESS calls. The first being done for no > good reason by the VFS, and the second being done in the OPEN call to > the server in the NFS client. Excuse me, but this is complete BS. First of all, are you suggesting that every filesystem out there must push the permission checks into its ->open()? This may_open() is where they normally happen... Moreover, the things like "this mount is r/o, don't let anything to be opened on it" are inherently client-side. It's a property of vfsmount; the same fs instance might be accessible r/w in another place. And things like opening a device node, etc. are not reaching the server at all. > > Merry Christmas > > Suggestions Al? Make nfs_permission() relax the checks when it sees MAY_OPEN, if you know that things will be caught by server anyway? -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 26, 2015 at 7:38 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Sat, Dec 26, 2015 at 07:11:16PM -0500, Trond Myklebust wrote: > >> Ah... IOW: That so called "fast path" crap in do_last() is screwing us >> over by forcing us to to 2 ACCESS calls. The first being done for no >> good reason by the VFS, and the second being done in the OPEN call to >> the server in the NFS client. > > Excuse me, but this is complete BS. First of all, are you suggesting > that every filesystem out there must push the permission checks into > its ->open()? This may_open() is where they normally happen... > > Moreover, the things like "this mount is r/o, don't let anything to > be opened on it" are inherently client-side. It's a property of > vfsmount; the same fs instance might be accessible r/w in another > place. And things like opening a device node, etc. are not reaching > the server at all. The may_open() is now happening before NFS gets a chance to issue the OPEN rpc call, which is a change w.r.t. the lookup intent code. The ordering is significant because it means the OPEN can no longer prime the access cache. >> > Merry Christmas >> >> Suggestions Al? > > Make nfs_permission() relax the checks when it sees MAY_OPEN, if you know > that things will be caught by server anyway? That can work as long as we're guaranteed that everything that calls inode_permission() with MAY_OPEN on a regular file will also follow up with a vfs_open() or dentry_open() on success. Is this always the case? -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index ce5a218..ffc25b0 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -2481,9 +2481,6 @@ force_lookup: res = PTR_ERR(cred); } out: - if (!res && (mask & MAY_EXEC) && !execute_ok(inode)) - res = -EACCES; - dfprintk(VFS, "NFS: permission(%s/%lu), mask=0x%x, res=%d\n", inode->i_sb->s_id, inode->i_ino, mask, res); return res;
This patch fixes a problem, that a nfs4 client incorrectly denies execute access based on outdated file mode (missing 'x' bit). After the mode on the server is 'fixed' (chmod +x) further execution attempts continue to fail, because the nfs ACCESS call updates the access parameter but not the mode parameter or the mode in the inode. The access check based on the file mode is not required, because the server already verified the clients rights. Remove the test. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109771 Signed-off-by: Donald Buczek <buczek@molgen.mpg.de> --- fs/nfs/dir.c | 3 --- 1 file changed, 3 deletions(-)