file locking fix for 3.2

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 <[email protected]>
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.

index 3b0d05d..96a487a 100644
@@ -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)

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?

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....)

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

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

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 <[email protected]>
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.

index 96a487a..0bd1745 100644
@@ -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;

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 <[email protected]>
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.

---
 fs/locks.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

index 3b0d05dcd7c1..637694bf3a03 100644
@@ -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;
 }
 

Looks fine to me.