file locking fix for 3.2

file locking fix for 3.2

[J. Bruce Fields]

Please pull the following change from the for-3.2 branch at:

	git://linux-nfs.org/~bfields/linux.git for-3.2

One small 3.2 regression in the file-locking code.  Seems like an
unlikely bug (null dereference on kmalloc failure), but the fix is a
one-liner.

--b.

J. Bruce Fields (1):
      locks: fix null dereference on lease-break failure path

 fs/locks.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

commit b67e18a8e3425725cc17cc7633bccb06fb3c9eda
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Mon Dec 19 17:57:11 2011 -0500

    locks: fix null dereference on lease-break failure path
    
    Commit 778fc546f749c588aa2f6cd50215d2715c374252 "locks: fix tracking of
    inprogress lease breaks" introduced a null dereference on failure to
    allocate memory.
    
    This means an open (without O_NONBLOCK set) on a file with a lease
    applied (generally only done when Samba or nfsd (with v4) is running)
    could crash if a kmalloc() fails.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/locks.c b/fs/locks.c
index 3b0d05d..96a487a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1214,8 +1214,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	if ((flock == NULL) || !IS_LEASE(flock))
 		goto out;
 
-	if (!locks_conflict(flock, new_fl))
-		goto out;
+	if (flock->fl_type == F_RDLCK && !want_write)
+		goto out; /* no conflict */
 
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next)
 		if (fl->fl_owner == current->files)

Re: file locking fix for 3.2

[Al Viro]

On Sat, Dec 24, 2011 at 04:50:12PM -0500, J. Bruce Fields wrote:

>     locks: fix null dereference on lease-break failure path
>     
>     Commit 778fc546f749c588aa2f6cd50215d2715c374252 "locks: fix tracking of
>     inprogress lease breaks" introduced a null dereference on failure to
>     allocate memory.
>     
>     This means an open (without O_NONBLOCK set) on a file with a lease
>     applied (generally only done when Samba or nfsd (with v4) is running)
>     could crash if a kmalloc() fails.

NULL?  AFAICS, lease_alloc() returns ERR_PTR() on failure...  I really
don't like the look of that code, TBH - at the very least it needs to
be commented a lot.  E.g. the rules for calling or not calling ->lm_break()
are really not obvious - AFAICS, we do that if
	i_have_this_lease || (mode & O_NONBLOCK)
is true *or* if allocation has succeeded.  The former condition is what'll
end up with -EWOULDBLOCK; I can understand not wanting to return that in
preference to -ENOMEM, but...  Do we want to skip ->lm_break() stuff only
in case of allocation failures that won't be overridden by -EWOULDBLOCK?

Re: file locking fix for 3.2

[J. Bruce Fields]

On Sat, Dec 24, 2011 at 10:55:25PM +0000, Al Viro wrote:
> On Sat, Dec 24, 2011 at 04:50:12PM -0500, J. Bruce Fields wrote:
> 
> >     locks: fix null dereference on lease-break failure path
> >     
> >     Commit 778fc546f749c588aa2f6cd50215d2715c374252 "locks: fix tracking of
> >     inprogress lease breaks" introduced a null dereference on failure to
> >     allocate memory.
> >     
> >     This means an open (without O_NONBLOCK set) on a file with a lease
> >     applied (generally only done when Samba or nfsd (with v4) is running)
> >     could crash if a kmalloc() fails.
> 
> NULL?  AFAICS, lease_alloc() returns ERR_PTR() on failure...

Erp, you're right.

(The fix is still right, it's the changelog that's wrong; happy to
fix and resend if it's wanted....)

> I really
> don't like the look of that code, TBH - at the very least it needs to
> be commented a lot.  E.g. the rules for calling or not calling ->lm_break()
> are really not obvious - AFAICS, we do that if
> 	i_have_this_lease || (mode & O_NONBLOCK)
> is true *or* if allocation has succeeded.  The former condition is what'll
> end up with -EWOULDBLOCK; I can understand not wanting to return that in
> preference to -ENOMEM, but...  Do we want to skip ->lm_break() stuff only
> in case of allocation failures that won't be overridden by -EWOULDBLOCK?

We do want to break leases at least in the O_NONBLOCK case so that a
caller can make forward progress by retrying open(.,O_NONBLOCK).

In the other cases I don't think there's any logic to the current
behavior.  Something like:

-	if (IS_ERR(new_fl) && !i_have_this_lease
-			&& ((mode & O_NONBLOCK) == 0)) {
-		error = PTR_ERR(new_fl);
-		goto out;
-	}
-
...
 		error = -EWOULDBLOCK;
 		goto out;
 	}
-
+	if (IS_ERR(new_fl)) {
+		error = PTR_ERR(new_fl);
+		goto out;
+	}
 restart:
 	break_time = flock->fl_break_time;
 	if (break_time != 0) {

would be a little less convoluted.

Or we could just do it the really obvious way:

 	new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
+	if (IS_ERR(new_fl))
+		return PTR_ERR(new_fl);
 
 	lock_flocks();
... 
-	if (IS_ERR(new_fl) && !i_have_this_lease
-			&& ((mode & O_NONBLOCK) == 0)) {
-		error = PTR_ERR(new_fl);
-		goto out;
-	}
-

Then you're returning -ENOMEM in a case when we really didn't need to do
an allocation, but is that really a problem?  It's a rare case, and
opens can already fail with -ENOMEM for other reasons, and I'd rather
not have the extra hair.

?

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: file locking fix for 3.2

[Al Viro]

On Sat, Dec 24, 2011 at 06:50:35PM -0500, J. Bruce Fields wrote:

> Then you're returning -ENOMEM in a case when we really didn't need to do
> an allocation, but is that really a problem?  It's a rare case, and
> opens can already fail with -ENOMEM for other reasons, and I'd rather
> not have the extra hair.

I'm certainly OK with that variant; if the folks maintaining fs/locks.c
are happy with it, I'd suggest going for it.  Note that you don't need
to touch locks_conflict() call at all if you bail out early on allocation
failure and it's definitely simpler and cleaner that way.

Re: file locking fix for 3.2

[J. Bruce Fields]

On Sun, Dec 25, 2011 at 12:05:42AM +0000, Al Viro wrote:
> On Sat, Dec 24, 2011 at 06:50:35PM -0500, J. Bruce Fields wrote:
> 
> > Then you're returning -ENOMEM in a case when we really didn't need to do
> > an allocation, but is that really a problem?  It's a rare case, and
> > opens can already fail with -ENOMEM for other reasons, and I'd rather
> > not have the extra hair.
> 
> I'm certainly OK with that variant; if the folks maintaining fs/locks.c

I've been more-or-less assuming that's me, not that I've been doing much
real maintenance to speak of.

> are happy with it, I'd suggest going for it.  Note that you don't need
> to touch locks_conflict() call at all if you bail out early on allocation
> failure and it's definitely simpler and cleaner that way.

Yep.

With no more -rc, and no chance to test anything myself till I'm back
from the holidays, my preference would be for Linus to merge the
already-posted one-liner.  Then I can queue up the below for 3.3.

--b.

commit 72acf27f6c20573d555d6b4450a7a9d41c4c9d5a
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Sun Dec 25 10:51:37 2011 -0700

    locks: simplify allocation in break_lease
    
    The code bends over backwards to avoid returning -ENOMEM in cases where
    the allocation wasn't really necessary.
    
    But there's nothing really *wrong* with returning -ENOMEM in those
    cases: break_lease callers can already return -ENOMEM for other reasons.
    So let's not take so much trouble over a rare case, and keep the code
    simpler.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/locks.c b/fs/locks.c
index 96a487a..0bd1745 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1205,6 +1205,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	int want_write = (mode & O_ACCMODE) != O_RDONLY;
 
 	new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
+	if (IS_ERR(new_fl))
+		return PTR_ERR(new_fl);
 
 	lock_flocks();
 
@@ -1214,19 +1216,13 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	if ((flock == NULL) || !IS_LEASE(flock))
 		goto out;
 
-	if (flock->fl_type == F_RDLCK && !want_write)
-		goto out; /* no conflict */
+	if (!locks_conflict(flock, new_fl))
+		goto out;
 
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next)
 		if (fl->fl_owner == current->files)
 			i_have_this_lease = 1;
 
-	if (IS_ERR(new_fl) && !i_have_this_lease
-			&& ((mode & O_NONBLOCK) == 0)) {
-		error = PTR_ERR(new_fl);
-		goto out;
-	}
-
 	break_time = 0;
 	if (lease_break_time > 0) {
 		break_time = jiffies + lease_break_time * HZ;

Re: file locking fix for 3.2

[Linus Torvalds]

I'm committing the appended version instead. It removes all the games with 
IS_ERR(), and just does the checking at the allocation point, the way the 
other lease_alloc() user does too.

Holler if you see something odd in there..

                   Linus
---
commit 6d4b9e38d3980826abccfbd90e95bf4bd41b8dd2
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Dec 26 10:25:26 2011 -0800

    vfs: fix handling of lock allocation failure in lease-break case
    
    Bruce Fields notes that commit 778fc546f749 ("locks: fix tracking of
    inprogress lease breaks") introduced a possible error pointer
    dereference on failure to allocate memory.  locks_conflict() will
    dereference the passed-in new lease lock structure that may be an error pointer.
    
    This means an open (without O_NONBLOCK set) on a file with a lease
    applied (generally only done when Samba or nfsd (with v4) is running)
    could crash if a kmalloc() fails.
    
    So instead of playing games with IS_ERROR() all over the place, just
    check the allocation failure early.  That makes the code more
    straightforward, and avoids this possible bad pointer dereference.
    
    Based-on-patch-by: J. Bruce Fields <bfields@redhat.com>
    Cc: Al Viro <viro@zeniv.linux.org.uk>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/locks.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 3b0d05dcd7c1..637694bf3a03 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1205,6 +1205,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	int want_write = (mode & O_ACCMODE) != O_RDONLY;
 
 	new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
+	if (IS_ERR(new_fl))
+		return PTR_ERR(new_fl);
 
 	lock_flocks();
 
@@ -1221,12 +1223,6 @@ int __break_lease(struct inode *inode, unsigned int mode)
 		if (fl->fl_owner == current->files)
 			i_have_this_lease = 1;
 
-	if (IS_ERR(new_fl) && !i_have_this_lease
-			&& ((mode & O_NONBLOCK) == 0)) {
-		error = PTR_ERR(new_fl);
-		goto out;
-	}
-
 	break_time = 0;
 	if (lease_break_time > 0) {
 		break_time = jiffies + lease_break_time * HZ;
@@ -1284,8 +1280,7 @@ restart:
 
 out:
 	unlock_flocks();
-	if (!IS_ERR(new_fl))
-		locks_free_lock(new_fl);
+	locks_free_lock(new_fl);
 	return error;
 }
 

Re: file locking fix for 3.2

[J. Bruce Fields]

On Mon, Dec 26, 2011 at 10:37:56AM -0800, Linus Torvalds wrote:
> 
> I'm committing the appended version instead. It removes all the games with 
> IS_ERR(), and just does the checking at the allocation point, the way the 
> other lease_alloc() user does too.
> 
> Holler if you see something odd in there..

Looks fine to me.

> @@ -1284,8 +1280,7 @@ restart:
>  
>  out:
>  	unlock_flocks();
> -	if (!IS_ERR(new_fl))
> -		locks_free_lock(new_fl);
> +	locks_free_lock(new_fl);
>  	return error;
>  }
>  

(And, oops, I missed that last check--thanks for catching it.)

--b.