| Message ID | cc80e246ee323a21f3f318248bf38eda668e8eb9.1449503713.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 07, 2015 at 11:26:01AM -0500, Benjamin Coddington wrote: > We only need to check lock exclusive/shared types against open mode when > flock() is used on NFS, so move it into the flock-specific path instead of > checking it for all locks. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/file.c | 15 +++++++++++++++ > fs/nfs/nfs4proc.c | 13 ------------- > 2 files changed, 15 insertions(+), 13 deletions(-) > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > index 93e2364..ec16abc 100644 > --- a/fs/nfs/file.c > +++ b/fs/nfs/file.c > @@ -893,6 +893,21 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) > /* We're simulating flock() locks using posix locks on the server */ > if (fl->fl_type == F_UNLCK) > return do_unlk(filp, cmd, fl, is_local); > + > + /* > + * Don't rely on the VFS having checked the file open mode, > + * since it won't do this for flock() locks. > + */ As this is only called for flock the comment doesn't make sense. And maybe it's also time to ask why the VFS doesn't do this, as I'd expect it to perform this instead of every file system. -- 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 Mon, 7 Dec 2015, Christoph Hellwig wrote: > On Mon, Dec 07, 2015 at 11:26:01AM -0500, Benjamin Coddington wrote: > > We only need to check lock exclusive/shared types against open mode when > > flock() is used on NFS, so move it into the flock-specific path instead of > > checking it for all locks. > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > fs/nfs/file.c | 15 +++++++++++++++ > > fs/nfs/nfs4proc.c | 13 ------------- > > 2 files changed, 15 insertions(+), 13 deletions(-) > > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > > index 93e2364..ec16abc 100644 > > --- a/fs/nfs/file.c > > +++ b/fs/nfs/file.c > > @@ -893,6 +893,21 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) > > /* We're simulating flock() locks using posix locks on the server */ > > if (fl->fl_type == F_UNLCK) > > return do_unlk(filp, cmd, fl, is_local); > > + > > + /* > > + * Don't rely on the VFS having checked the file open mode, > > + * since it won't do this for flock() locks. > > + */ > > As this is only called for flock the comment doesn't make sense. And > maybe it's also time to ask why the VFS doesn't do this, as I'd expect > it to perform this instead of every file system. I can fixup the comment for clarity as I just moved this chunk over. I'm not aware that flock() has ever had this check, but posix locks requires it. My understanding is that since NFS may simulate flock() with posix locking, the check is necessary for NFS. I can only speculate what applications out there may be using mis-matched file modes and flock operations. Changing that behavior seems beyond the scope of this work. Ben -- 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 Mon, 7 Dec 2015 10:40:36 -0800 Christoph Hellwig <hch@infradead.org> wrote: > On Mon, Dec 07, 2015 at 11:26:01AM -0500, Benjamin Coddington wrote: > > We only need to check lock exclusive/shared types against open mode when > > flock() is used on NFS, so move it into the flock-specific path instead of > > checking it for all locks. > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > fs/nfs/file.c | 15 +++++++++++++++ > > fs/nfs/nfs4proc.c | 13 ------------- > > 2 files changed, 15 insertions(+), 13 deletions(-) > > > > diff --git a/fs/nfs/file.c b/fs/nfs/file.c > > index 93e2364..ec16abc 100644 > > --- a/fs/nfs/file.c > > +++ b/fs/nfs/file.c > > @@ -893,6 +893,21 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) > > /* We're simulating flock() locks using posix locks on the server */ > > if (fl->fl_type == F_UNLCK) > > return do_unlk(filp, cmd, fl, is_local); > > + > > + /* > > + * Don't rely on the VFS having checked the file open mode, > > + * since it won't do this for flock() locks. > > + */ > > As this is only called for flock the comment doesn't make sense. And > maybe it's also time to ask why the VFS doesn't do this, as I'd expect > it to perform this instead of every file system. IIRC, flock doesn't require this check. You can (for instance) open a file for read and lock it for write. POSIX locks (and NFSv4 locks, by extension) don't allow that. Since we're mapping flock locks onto v4 locks here, we have to do that check in the NFS code.
On Mon, Dec 07, 2015 at 02:13:47PM -0500, Benjamin Coddington wrote: > I can fixup the comment for clarity as I just moved this chunk over. > > I'm not aware that flock() has ever had this check, but posix locks requires > it. My understanding is that since NFS may simulate flock() with posix > locking, the check is necessary for NFS. Ah, right. Maybe that's what should be in the comment here.. -- 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 Mon, 7 Dec 2015, Christoph Hellwig wrote: > On Mon, Dec 07, 2015 at 02:13:47PM -0500, Benjamin Coddington wrote: > > I can fixup the comment for clarity as I just moved this chunk over. > > > > I'm not aware that flock() has ever had this check, but posix locks requires > > it. My understanding is that since NFS may simulate flock() with posix > > locking, the check is necessary for NFS. > > Ah, right. Maybe that's what should be in the comment here.. Yes, good idea. That's where someone is going to look when they investigate why their application breaks on NFS. Ben -- 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 Mon, Dec 7, 2015 at 2:24 PM, Benjamin Coddington <bcodding@redhat.com> wrote: > On Mon, 7 Dec 2015, Christoph Hellwig wrote: > >> On Mon, Dec 07, 2015 at 02:13:47PM -0500, Benjamin Coddington wrote: >> > I can fixup the comment for clarity as I just moved this chunk over. >> > >> > I'm not aware that flock() has ever had this check, but posix locks requires >> > it. My understanding is that since NFS may simulate flock() with posix >> > locking, the check is necessary for NFS. >> >> Ah, right. Maybe that's what should be in the comment here.. > > Yes, good idea. That's where someone is going to look when they investigate > why their application breaks on NFS. > Should I expect a resend? 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 Mon, Dec 7, 2015 at 2:24 PM, Benjamin Coddington <bcodding@redhat.com> wrote: > > On Mon, 7 Dec 2015, Christoph Hellwig wrote: > > > >> On Mon, Dec 07, 2015 at 02:13:47PM -0500, Benjamin Coddington wrote: > >> > I can fixup the comment for clarity as I just moved this chunk over. > >> > > >> > I'm not aware that flock() has ever had this check, but posix locks requires > >> > it. My understanding is that since NFS may simulate flock() with posix > >> > locking, the check is necessary for NFS. > >> > >> Ah, right. Maybe that's what should be in the comment here.. > > > > Yes, good idea. That's where someone is going to look when they investigate > > why their application breaks on NFS. > > > > Should I expect a resend? I'll do this in a v3 of this shortly. Ben -- 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/file.c b/fs/nfs/file.c index 93e2364..ec16abc 100644 --- a/fs/nfs/file.c +++ b/fs/nfs/file.c @@ -893,6 +893,21 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl) /* We're simulating flock() locks using posix locks on the server */ if (fl->fl_type == F_UNLCK) return do_unlk(filp, cmd, fl, is_local); + + /* + * Don't rely on the VFS having checked the file open mode, + * since it won't do this for flock() locks. + */ + switch (fl->fl_type) { + case F_RDLCK: + if (!(filp->f_mode & FMODE_READ)) + return -EBADF; + break; + case F_WRLCK: + if (!(filp->f_mode & FMODE_WRITE)) + return -EBADF; + } + return do_setlk(filp, cmd, fl, is_local); } EXPORT_SYMBOL_GPL(nfs_flock); diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 2231d7d..05ea1e1 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -6125,19 +6125,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request) if (state == NULL) return -ENOLCK; - /* - * Don't rely on the VFS having checked the file open mode, - * since it won't do this for flock() locks. - */ - switch (request->fl_type) { - case F_RDLCK: - if (!(filp->f_mode & FMODE_READ)) - return -EBADF; - break; - case F_WRLCK: - if (!(filp->f_mode & FMODE_WRITE)) - return -EBADF; - } do { status = nfs4_proc_setlk(state, cmd, request);
We only need to check lock exclusive/shared types against open mode when flock() is used on NFS, so move it into the flock-specific path instead of checking it for all locks. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/file.c | 15 +++++++++++++++ fs/nfs/nfs4proc.c | 13 ------------- 2 files changed, 15 insertions(+), 13 deletions(-)