Re: F_RDLCK lock to FreeBSD NFS server fails to R/O target file (nasty little bug)
John wrote:
> After pondering the best way to allow the VOP_ACCESS() call to
> only query for the permissions really needed, I've come up with
> a patch that minimally adds one parameter to the nlm_get_vfs_state()
> function call with the lock type from the original argp.
>
> http://people.freebsd.org/~jwd/nlm_prot_impl.c.accmode.patch
>
> I'd appreciate a review and seeing what might be required to commit
> this prior to 9 release.
>
Doing a little testing with your patch, I came across this nasty little
bug.
I have a little program that does the following:
- opens a file read/write
- waits for user input
- does read and write locks (non overlapping byte ranges)
- does an unlock of all bytes covered by the above locks
I ran it as follows:
- had a file on the NFSv3 mount point (which is using the NLM)
not owned by the user running the above program, with file
mode 0666
- the program opens the file read/write and waits for user input
--> on the server, I did a "chmod 622 "
- gave it user input, so it then did the above locks
(with your patch, the read lock fails although the read lock
works for the unpatched NLM and my variant of the patch)
- it also does the unlock and doesn't complain of a failure.
---> The ugly bug is that, for your patch, the unlock has failed
silently.
If you subsequently try and write lock a byte range overlapping
the write lock byte range above, the attempt fails, since it is
still locked by the client.
---> I don't know of an easy way to get rid of that lock!!!
(It persists after the client unmounts the NFS file system.)
Cute, eh:-)
Btw, I suspect you can get the unpatched code to fail the same way
if you did a "chmod 600 " after the write lock, but before
the unlock.
I'm almost thinking that an unlock shouldn't do a VOP_ACCESS() of
any kind, but since that isn't current behaviour???
rick
> Thanks,
> John
>
> - John's Original Message -
> > Hi Fellow NFS'ers,
> >
> >I believe I have found the problem we've been having with read
> >locks
> > while attaching to a FreeBSD NFS server.
> >
> >In sys/nlm/nlm_prot_impl.c, function nlm_get_vfs_state(), there
> >is a call
> > to VOP_ACCESS() as follows:
> >
> > /*
> > * Check cred.
> > */
> > NLM_DEBUG(3, "nlm_get_vfs_state(): Calling
> > VOP_ACCESS(VWRITE) with cred->cr_uid=%d\n",cred->cr_uid);
> > error = VOP_ACCESS(vs->vs_vp, VWRITE, cred, curthread);
> > if (error) {
> > NLM_DEBUG(3, "nlm_get_vfs_state(): caller_name = %s
> > VOP_ACCESS() returns %d\n",
> > host->nh_caller_name, error);
> > goto out;
> > }
> >
> > The file being accessed is read only to the user, and open()ed
> > with
> > O_RDONLY. The lock being requested is for a read.
> >
> > fd = open(filename, O_RDONLY, 0);
> > ...
> >
> > lblk.l_type = F_RDLCK;
> > lblk.l_start = 0;
> > lblk.l_whence= SEEK_SET;
> > lblk.l_len = 0;
> > lblk.l_pid = 0;
> > rc = fcntl(fd, F_SETLK, &lblk);
> >
> >Running the above from a remote system, the lock call fails with
> > errno set to ENOLCK. Given cred->cr_uid comes in as 227 which is
> > my uid on the remote system. Since the file is R/O to me, and the
> > VOP_ACCESS() is asking for VWRITE, it fails with errno 13, EACCES,
> > Permission denied.
> >
> >The above operations work correctly to some of our other
> > favorite big-name nfs vendors :-)
> >
> >Opinions on the "correct" way to fix this?
> >
> > 1. Since we're only asking for a read lock, why do we need to ask
> >for VWRITE? I may not understand an underlying requirement for
> >the VWRITE so please feel free to educate me if needed.
> >
> >Something like: request == F_RDLCK ? VREAD : VWRITE
> >(need to figure out where to get the request from in this
> >context).
> >
> > 2. Attempt VWRITE, fallback to VREAD... seems off to me though.
> >
> > 3. Other?
> >
> >I appreciate any thoughts on this.
> >
> > Thanks,
> > John
> >
> >While they might not follow style(9) completely, I've uploaded
> > my patch to nlm_prot.impl.c with the NLM_DEBUG() calls i've added.
> > I'd appreciate it if someone would consider committing them so
> > who ever debugs this file next will have them available.
> >
> > http://people.freebsd.org/~jwd/nlm_prot_impl.c.patch
> >
> ___
> freebsd...@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscr...@freebsd.org"
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: F_RDLCK lock to FreeBSD NFS server fails to R/O target file [PATCH]
John wrote:
> After pondering the best way to allow the VOP_ACCESS() call to
> only query for the permissions really needed, I've come up with
> a patch that minimally adds one parameter to the nlm_get_vfs_state()
> function call with the lock type from the original argp.
>
> http://people.freebsd.org/~jwd/nlm_prot_impl.c.accmode.patch
>
> I'd appreciate a review and seeing what might be required to commit
> this prior to 9 release.
>
> Thanks,
> John
>
> - John's Original Message -
> > Hi Fellow NFS'ers,
> >
> >I believe I have found the problem we've been having with read
> >locks
> > while attaching to a FreeBSD NFS server.
> >
> >In sys/nlm/nlm_prot_impl.c, function nlm_get_vfs_state(), there
> >is a call
> > to VOP_ACCESS() as follows:
> >
> > /*
> > * Check cred.
> > */
> > NLM_DEBUG(3, "nlm_get_vfs_state(): Calling
> > VOP_ACCESS(VWRITE) with cred->cr_uid=%d\n",cred->cr_uid);
> > error = VOP_ACCESS(vs->vs_vp, VWRITE, cred, curthread);
> > if (error) {
> > NLM_DEBUG(3, "nlm_get_vfs_state(): caller_name = %s
> > VOP_ACCESS() returns %d\n",
> > host->nh_caller_name, error);
> > goto out;
> > }
> >
> > The file being accessed is read only to the user, and open()ed
> > with
> > O_RDONLY. The lock being requested is for a read.
> >
> > fd = open(filename, O_RDONLY, 0);
> > ...
> >
> > lblk.l_type = F_RDLCK;
> > lblk.l_start = 0;
> > lblk.l_whence= SEEK_SET;
> > lblk.l_len = 0;
> > lblk.l_pid = 0;
> > rc = fcntl(fd, F_SETLK, &lblk);
> >
> >Running the above from a remote system, the lock call fails with
> > errno set to ENOLCK. Given cred->cr_uid comes in as 227 which is
> > my uid on the remote system. Since the file is R/O to me, and the
> > VOP_ACCESS() is asking for VWRITE, it fails with errno 13, EACCES,
> > Permission denied.
> >
> >The above operations work correctly to some of our other
> > favorite big-name nfs vendors :-)
> >
> >Opinions on the "correct" way to fix this?
> >
> > 1. Since we're only asking for a read lock, why do we need to ask
> >for VWRITE? I may not understand an underlying requirement for
> >the VWRITE so please feel free to educate me if needed.
> >
> >Something like: request == F_RDLCK ? VREAD : VWRITE
> >(need to figure out where to get the request from in this
> >context).
> >
> > 2. Attempt VWRITE, fallback to VREAD... seems off to me though.
> >
I think I prefer this approach because it will never fail unless it
would fail without the patch. For the case where the client uid has
write but not read permission and attempts an F_RDLCK, I believe your
patch would fail the request whereas it will succeed without your patch.
(Although I'll admit I didn't actually test this.;-)
Doing VOP_ACCESS(..VWRITE..) first and only doing a VOP_ACCESS(..VREAD..)
if the VOP_ACCESS(..VWRITE..) fails will preserve current behaviour fot
the non-failing case, but allow an F_RDLCK for uids with read-only access.
I've attached a patch that does this variant. John, could you test this
patch?
And does anyone have an opinion w.r.t. which variant of the patch is
more appropriate?
Thanks in advance for your help, rick
> > 3. Other?
> >
> >I appreciate any thoughts on this.
> >
> > Thanks,
> > John
> >
> >While they might not follow style(9) completely, I've uploaded
> > my patch to nlm_prot.impl.c with the NLM_DEBUG() calls i've added.
> > I'd appreciate it if someone would consider committing them so
> > who ever debugs this file next will have them available.
> >
> > http://people.freebsd.org/~jwd/nlm_prot_impl.c.patch
> >
> ___
> freebsd...@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscr...@freebsd.org"
--- nlm/nlm_prot_impl.c.sav 2011-12-05 10:59:00.0 -0500
+++ nlm/nlm_prot_impl.c 2011-12-05 20:09:03.0 -0500
@@ -1774,7 +1774,7 @@ struct vfs_state {
static int
nlm_get_vfs_state(struct nlm_host *host, struct svc_req *rqstp,
-fhandle_t *fhp, struct vfs_state *vs)
+fhandle_t *fhp, struct vfs_state *vs, bool_t exclusive)
{
int error, exflags;
struct ucred *cred = NULL, *credanon;
@@ -1816,6 +1816,8 @@ nlm_get_vfs_state(struct nlm_host *host,
* Check cred.
*/
error = VOP_ACCESS(vs->vs_vp, VWRITE, cred, curthread);
+ if (error != 0 && !exclusive)
+ error = VOP_ACCESS(vs->vs_vp, VREAD, cred, curthread);
if (error)
goto out;
@@ -1896,7 +1898,7 @@ nlm_do_test(nlm4_testargs *argp, nlm4_te
goto out;
}
- error = nlm_get_vfs_state(host, rqstp, &fh, &vs);
+ error = nlm_get_vfs_state(host, rqstp, &fh, &vs, argp->exclusive);
if (error) {
result->stat.stat = nlm_convert_error(error);
goto out;
@@ -2001,7 +2003,7 @@ nlm_do_lock(nlm4_lockargs *argp, nlm4_re
goto out;
}
- error = nlm_get_vfs_state(host, rqstp, &fh,
Re: F_RDLCK lock to FreeBSD NFS server fails to R/O target file [PATCH]
John wrote:
> After pondering the best way to allow the VOP_ACCESS() call to
> only query for the permissions really needed, I've come up with
> a patch that minimally adds one parameter to the nlm_get_vfs_state()
> function call with the lock type from the original argp.
>
> http://people.freebsd.org/~jwd/nlm_prot_impl.c.accmode.patch
>
I took a look at it and it seemed mostly ok. However, please note that
I am not familiar with the NLM code and try to avoid it like the plague.:-)
One place I would suggest might want to be changed is the nlm_do_unlock()
case. I don't think any file permission checking is needed for an unlock
and it seems like it might fail when the called has VWRITE but not VREAD
permissions on the file? Leaving a file locked is not a good situation.
I would just not even do the VOP_ACCESS() call for that case.
Maybe pass accmode == 0 into nlm_get_vfs_state() to indicate "skip the
VOP_ACCESS() call".
I think that this patch might be a little risky to put into head at this
stage of the release cycle so, personally, I'd wait until after the 9.0
release before I'd look at committing it. Others might feel it's ok to
go in now?
rick
> I'd appreciate a review and seeing what might be required to commit
> this prior to 9 release.
>
> Thanks,
> John
>
> - John's Original Message -
> > Hi Fellow NFS'ers,
> >
> >I believe I have found the problem we've been having with read
> >locks
> > while attaching to a FreeBSD NFS server.
> >
> >In sys/nlm/nlm_prot_impl.c, function nlm_get_vfs_state(), there
> >is a call
> > to VOP_ACCESS() as follows:
> >
> > /*
> > * Check cred.
> > */
> > NLM_DEBUG(3, "nlm_get_vfs_state(): Calling
> > VOP_ACCESS(VWRITE) with cred->cr_uid=%d\n",cred->cr_uid);
> > error = VOP_ACCESS(vs->vs_vp, VWRITE, cred, curthread);
> > if (error) {
> > NLM_DEBUG(3, "nlm_get_vfs_state(): caller_name = %s
> > VOP_ACCESS() returns %d\n",
> > host->nh_caller_name, error);
> > goto out;
> > }
> >
> > The file being accessed is read only to the user, and open()ed
> > with
> > O_RDONLY. The lock being requested is for a read.
> >
> > fd = open(filename, O_RDONLY, 0);
> > ...
> >
> > lblk.l_type = F_RDLCK;
> > lblk.l_start = 0;
> > lblk.l_whence= SEEK_SET;
> > lblk.l_len = 0;
> > lblk.l_pid = 0;
> > rc = fcntl(fd, F_SETLK, &lblk);
> >
> >Running the above from a remote system, the lock call fails with
> > errno set to ENOLCK. Given cred->cr_uid comes in as 227 which is
> > my uid on the remote system. Since the file is R/O to me, and the
> > VOP_ACCESS() is asking for VWRITE, it fails with errno 13, EACCES,
> > Permission denied.
> >
> >The above operations work correctly to some of our other
> > favorite big-name nfs vendors :-)
> >
> >Opinions on the "correct" way to fix this?
> >
> > 1. Since we're only asking for a read lock, why do we need to ask
> >for VWRITE? I may not understand an underlying requirement for
> >the VWRITE so please feel free to educate me if needed.
> >
> >Something like: request == F_RDLCK ? VREAD : VWRITE
> >(need to figure out where to get the request from in this
> >context).
> >
> > 2. Attempt VWRITE, fallback to VREAD... seems off to me though.
> >
> > 3. Other?
> >
> >I appreciate any thoughts on this.
> >
> > Thanks,
> > John
> >
> >While they might not follow style(9) completely, I've uploaded
> > my patch to nlm_prot.impl.c with the NLM_DEBUG() calls i've added.
> > I'd appreciate it if someone would consider committing them so
> > who ever debugs this file next will have them available.
> >
> > http://people.freebsd.org/~jwd/nlm_prot_impl.c.patch
> >
> ___
> freebsd...@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscr...@freebsd.org"
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: F_RDLCK lock to FreeBSD NFS server fails to R/O target file [PATCH]
After pondering the best way to allow the VOP_ACCESS() call to
only query for the permissions really needed, I've come up with
a patch that minimally adds one parameter to the nlm_get_vfs_state()
function call with the lock type from the original argp.
http://people.freebsd.org/~jwd/nlm_prot_impl.c.accmode.patch
I'd appreciate a review and seeing what might be required to commit
this prior to 9 release.
Thanks,
John
- John's Original Message -
> Hi Fellow NFS'ers,
>
>I believe I have found the problem we've been having with read locks
> while attaching to a FreeBSD NFS server.
>
>In sys/nlm/nlm_prot_impl.c, function nlm_get_vfs_state(), there is a call
> to VOP_ACCESS() as follows:
>
> /*
> * Check cred.
> */
> NLM_DEBUG(3, "nlm_get_vfs_state(): Calling VOP_ACCESS(VWRITE) with
> cred->cr_uid=%d\n",cred->cr_uid);
> error = VOP_ACCESS(vs->vs_vp, VWRITE, cred, curthread);
> if (error) {
> NLM_DEBUG(3, "nlm_get_vfs_state(): caller_name = %s
> VOP_ACCESS() returns %d\n",
> host->nh_caller_name, error);
> goto out;
> }
>
> The file being accessed is read only to the user, and open()ed with
> O_RDONLY. The lock being requested is for a read.
>
> fd = open(filename, O_RDONLY, 0);
> ...
>
> lblk.l_type = F_RDLCK;
> lblk.l_start = 0;
> lblk.l_whence= SEEK_SET;
> lblk.l_len = 0;
> lblk.l_pid = 0;
> rc = fcntl(fd, F_SETLK, &lblk);
>
>Running the above from a remote system, the lock call fails with
> errno set to ENOLCK. Given cred->cr_uid comes in as 227 which is
> my uid on the remote system. Since the file is R/O to me, and the
> VOP_ACCESS() is asking for VWRITE, it fails with errno 13, EACCES,
> Permission denied.
>
>The above operations work correctly to some of our other
> favorite big-name nfs vendors :-)
>
>Opinions on the "correct" way to fix this?
>
> 1. Since we're only asking for a read lock, why do we need to ask
>for VWRITE? I may not understand an underlying requirement for
>the VWRITE so please feel free to educate me if needed.
>
>Something like: request == F_RDLCK ? VREAD : VWRITE
>(need to figure out where to get the request from in this context).
>
> 2. Attempt VWRITE, fallback to VREAD... seems off to me though.
>
> 3. Other?
>
>I appreciate any thoughts on this.
>
> Thanks,
> John
>
>While they might not follow style(9) completely, I've uploaded
> my patch to nlm_prot.impl.c with the NLM_DEBUG() calls i've added.
> I'd appreciate it if someone would consider committing them so
> who ever debugs this file next will have them available.
>
> http://people.freebsd.org/~jwd/nlm_prot_impl.c.patch
>
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
F_RDLCK lock to FreeBSD NFS server fails to R/O target file
Hi Fellow NFS'ers,
I believe I have found the problem we've been having with read locks
while attaching to a FreeBSD NFS server.
In sys/nlm/nlm_prot_impl.c, function nlm_get_vfs_state(), there is a call
to VOP_ACCESS() as follows:
/*
* Check cred.
*/
NLM_DEBUG(3, "nlm_get_vfs_state(): Calling VOP_ACCESS(VWRITE) with
cred->cr_uid=%d\n",cred->cr_uid);
error = VOP_ACCESS(vs->vs_vp, VWRITE, cred, curthread);
if (error) {
NLM_DEBUG(3, "nlm_get_vfs_state(): caller_name = %s
VOP_ACCESS() returns %d\n",
host->nh_caller_name, error);
goto out;
}
The file being accessed is read only to the user, and open()ed with
O_RDONLY. The lock being requested is for a read.
fd = open(filename, O_RDONLY, 0);
...
lblk.l_type = F_RDLCK;
lblk.l_start = 0;
lblk.l_whence= SEEK_SET;
lblk.l_len = 0;
lblk.l_pid = 0;
rc = fcntl(fd, F_SETLK, &lblk);
Running the above from a remote system, the lock call fails with
errno set to ENOLCK. Given cred->cr_uid comes in as 227 which is
my uid on the remote system. Since the file is R/O to me, and the
VOP_ACCESS() is asking for VWRITE, it fails with errno 13, EACCES,
Permission denied.
The above operations work correctly to some of our other
favorite big-name nfs vendors :-)
Opinions on the "correct" way to fix this?
1. Since we're only asking for a read lock, why do we need to ask
for VWRITE? I may not understand an underlying requirement for
the VWRITE so please feel free to educate me if needed.
Something like: request == F_RDLCK ? VREAD : VWRITE
(need to figure out where to get the request from in this context).
2. Attempt VWRITE, fallback to VREAD... seems off to me though.
3. Other?
I appreciate any thoughts on this.
Thanks,
John
While they might not follow style(9) completely, I've uploaded
my patch to nlm_prot.impl.c with the NLM_DEBUG() calls i've added.
I'd appreciate it if someone would consider committing them so
who ever debugs this file next will have them available.
http://people.freebsd.org/~jwd/nlm_prot_impl.c.patch
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
