[PATCH] locks: only validate the lock vs. f_mode in F_SETLK codepaths

[PATCH] locks: only validate the lock vs. f_mode in F_SETLK codepaths

[Jeff Layton]

commit bce7560d4946 (locks: consolidate checks for compatible
filp->f_mode values in setlk handlers) introduced a regression in the
F_GETLK handler.

flock64_to_posix_lock is a shared codepath between F_GETLK and F_SETLK,
but the f_mode checks should only be applicable to the F_SETLK codepaths
according to POSIX.

Instead of just reverting the patch, add a new function to do this
checking and have the F_SETLK handlers call it.

Reported-by: Reuben Farrelly <reuben@reub.net>
Signed-off-by: Jeff Layton <jlayton@poochiereds.net>
---
 fs/locks.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index e663aeac579e..ea688edde911 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -389,18 +389,6 @@ static int flock64_to_posix_lock(struct file *filp, struct file_lock *fl,
 	fl->fl_ops = NULL;
 	fl->fl_lmops = NULL;
 
-	/* Ensure that fl->fl_filp has compatible f_mode */
-	switch (l->l_type) {
-	case F_RDLCK:
-		if (!(filp->f_mode & FMODE_READ))
-			return -EBADF;
-		break;
-	case F_WRLCK:
-		if (!(filp->f_mode & FMODE_WRITE))
-			return -EBADF;
-		break;
-	}
-
 	return assign_type(fl, l->l_type);
 }
 
@@ -2034,6 +2022,21 @@ static int do_lock_file_wait(struct file *filp, unsigned int cmd,
 	return error;
 }
 
+/* Ensure that fl->fl_filp has compatible f_mode for F_SETLK calls */
+static int
+check_fmode_for_setlk(struct file_lock *fl)
+{
+	switch (fl->fl_type) {
+	case F_RDLCK:
+		if (!(fl->fl_file->f_mode & FMODE_READ))
+			return -EBADF;
+	case F_WRLCK:
+		if (!(fl->fl_file->f_mode & FMODE_WRITE))
+			return -EBADF;
+	}
+	return 0;
+}
+
 /* Apply the lock described by l to an open file descriptor.
  * This implements both the F_SETLK and F_SETLKW commands of fcntl().
  */
@@ -2071,6 +2074,10 @@ again:
 	if (error)
 		goto out;
 
+	error = check_fmode_for_setlk(file_lock);
+	if (error)
+		goto out;
+
 	/*
 	 * If the cmd is requesting file-private locks, then set the
 	 * FL_OFDLCK flag and override the owner.
@@ -2206,6 +2213,10 @@ again:
 	if (error)
 		goto out;
 
+	error = check_fmode_for_setlk(file_lock);
+	if (error)
+		goto out;
+
 	/*
 	 * If the cmd is requesting file-private locks, then set the
 	 * FL_OFDLCK flag and override the owner.
-- 
1.9.0

Re: [PATCH] locks: only validate the lock vs. f_mode in F_SETLK codepaths

[Dave Jones]

On Fri, May 09, 2014 at 12:21:29PM -0400, Jeff Layton wrote:

 > -	/* Ensure that fl->fl_filp has compatible f_mode */
 > -	switch (l->l_type) {
 > -	case F_RDLCK:
 > -		if (!(filp->f_mode & FMODE_READ))
 > -			return -EBADF;
 > -		break;
 > -	case F_WRLCK:
 > -		if (!(filp->f_mode & FMODE_WRITE))
 > -			return -EBADF;
 > -		break;
 > -	}


 > +check_fmode_for_setlk(struct file_lock *fl)
 > +{
 > +	switch (fl->fl_type) {
 > +	case F_RDLCK:
 > +		if (!(fl->fl_file->f_mode & FMODE_READ))
 > +			return -EBADF;
 > +	case F_WRLCK:
 > +		if (!(fl->fl_file->f_mode & FMODE_WRITE))
 > +			return -EBADF;
 > +	}
 > +	return 0;
 > +}

Why are we now checking FMODE_WRITE for the RDLCK case ?
Or was losing the break; unintentional ?

	Dave

Re: [PATCH] locks: only validate the lock vs. f_mode in F_SETLK codepaths

[Jeff Layton]

On Fri, 9 May 2014 14:07:44 -0400
Dave Jones <davej@redhat.com> wrote:

> On Fri, May 09, 2014 at 12:21:29PM -0400, Jeff Layton wrote:
> 
>  > -	/* Ensure that fl->fl_filp has compatible f_mode */
>  > -	switch (l->l_type) {
>  > -	case F_RDLCK:
>  > -		if (!(filp->f_mode & FMODE_READ))
>  > -			return -EBADF;
>  > -		break;
>  > -	case F_WRLCK:
>  > -		if (!(filp->f_mode & FMODE_WRITE))
>  > -			return -EBADF;
>  > -		break;
>  > -	}
> 
> 
>  > +check_fmode_for_setlk(struct file_lock *fl)
>  > +{
>  > +	switch (fl->fl_type) {
>  > +	case F_RDLCK:
>  > +		if (!(fl->fl_file->f_mode & FMODE_READ))
>  > +			return -EBADF;
>  > +	case F_WRLCK:
>  > +		if (!(fl->fl_file->f_mode & FMODE_WRITE))
>  > +			return -EBADF;
>  > +	}
>  > +	return 0;
>  > +}
> 
> Why are we now checking FMODE_WRITE for the RDLCK case ?
> Or was losing the break; unintentional ?
> 
> 	Dave
> 

Well spotted -- that was indeed unintentional. Fixed in my repo and in
the code in -next, but I'll refrain from re-posting for now.

Clearly, I need to roll some better file locking tests... ;)

Thanks,
-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] locks: only validate the lock vs. f_mode in F_SETLK codepaths

[Reuben Farrelly]

On 10/05/2014 4:15 AM, Jeff Layton wrote:
> Well spotted -- that was indeed unintentional. Fixed in my repo and in
> the code in -next, but I'll refrain from re-posting for now.
>
> Clearly, I need to roll some better file locking tests... ;)
>
> Thanks,

I've just applied the following 3 patches from Jeff's git repo on top of 
a vanilla 3.15-rc5:

09e558d9baa6416111bce8610c6bbd9a525ee057 [PATCH] locks: only validate 
the lock vs. f_mode in F_SETLK codepaths

e4bf5e2a931bdf053e5bcb72ec959ee151ef2aaf [PATCH] locks: ensure that 
fl_owner is always initialized properly in flock and lease codepaths

0e71bcb3ec5d8fba7e534d379ad69933ed6493e0 [PATCH] fs/locks.c: replace 
seq_printf by seq_puts

I can confirm that this fixes the problem - in that Samba now compiles 
through to completion.

Thanks for the fix!

Reuben

[PATCH v2] locks: only validate the lock vs. f_mode in F_SETLK codepaths

[Jeff Layton]

v2: replace missing break in switch statement (as pointed out by Dave
    Jones)

commit bce7560d4946 (locks: consolidate checks for compatible
filp->f_mode values in setlk handlers) introduced a regression in the
F_GETLK handler.

flock64_to_posix_lock is a shared codepath between F_GETLK and F_SETLK,
but the f_mode checks should only be applicable to the F_SETLK codepaths
according to POSIX.

Instead of just reverting the patch, add a new function to do this
checking and have the F_SETLK handlers call it.

Cc: Dave Jones <davej@redhat.com>
Reported-and-Tested-by: Reuben Farrelly <reuben@reub.net>
Signed-off-by: Jeff Layton <jlayton@poochiereds.net>
---
 fs/locks.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index e663aeac579e..e390bd9ae068 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -389,18 +389,6 @@ static int flock64_to_posix_lock(struct file *filp, struct file_lock *fl,
 	fl->fl_ops = NULL;
 	fl->fl_lmops = NULL;
 
-	/* Ensure that fl->fl_filp has compatible f_mode */
-	switch (l->l_type) {
-	case F_RDLCK:
-		if (!(filp->f_mode & FMODE_READ))
-			return -EBADF;
-		break;
-	case F_WRLCK:
-		if (!(filp->f_mode & FMODE_WRITE))
-			return -EBADF;
-		break;
-	}
-
 	return assign_type(fl, l->l_type);
 }
 
@@ -2034,6 +2022,22 @@ static int do_lock_file_wait(struct file *filp, unsigned int cmd,
 	return error;
 }
 
+/* Ensure that fl->fl_filp has compatible f_mode for F_SETLK calls */
+static int
+check_fmode_for_setlk(struct file_lock *fl)
+{
+	switch (fl->fl_type) {
+	case F_RDLCK:
+		if (!(fl->fl_file->f_mode & FMODE_READ))
+			return -EBADF;
+		break;
+	case F_WRLCK:
+		if (!(fl->fl_file->f_mode & FMODE_WRITE))
+			return -EBADF;
+	}
+	return 0;
+}
+
 /* Apply the lock described by l to an open file descriptor.
  * This implements both the F_SETLK and F_SETLKW commands of fcntl().
  */
@@ -2071,6 +2075,10 @@ again:
 	if (error)
 		goto out;
 
+	error = check_fmode_for_setlk(file_lock);
+	if (error)
+		goto out;
+
 	/*
 	 * If the cmd is requesting file-private locks, then set the
 	 * FL_OFDLCK flag and override the owner.
@@ -2206,6 +2214,10 @@ again:
 	if (error)
 		goto out;
 
+	error = check_fmode_for_setlk(file_lock);
+	if (error)
+		goto out;
+
 	/*
 	 * If the cmd is requesting file-private locks, then set the
 	 * FL_OFDLCK flag and override the owner.
-- 
1.9.0