* file locking fix for 3.2
@ 2011-12-24 21:50 J. Bruce Fields
2011-12-24 22:55 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: J. Bruce Fields @ 2011-12-24 21:50 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-nfs, linux-fsdevel, linux-kernel
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)
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: file locking fix for 3.2
2011-12-24 21:50 file locking fix for 3.2 J. Bruce Fields
@ 2011-12-24 22:55 ` Al Viro
[not found] ` <20111224225525.GR23916-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2011-12-24 22:55 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Linus Torvalds, linux-nfs, linux-fsdevel, linux-kernel
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: file locking fix for 3.2
[not found] ` <20111224225525.GR23916-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
@ 2011-12-24 23:50 ` J. Bruce Fields
2011-12-25 0:05 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: J. Bruce Fields @ 2011-12-24 23:50 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: file locking fix for 3.2
2011-12-24 23:50 ` J. Bruce Fields
@ 2011-12-25 0:05 ` Al Viro
2011-12-25 18:19 ` J. Bruce Fields
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2011-12-25 0:05 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Linus Torvalds, linux-nfs, linux-fsdevel, linux-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: file locking fix for 3.2
2011-12-25 0:05 ` Al Viro
@ 2011-12-25 18:19 ` J. Bruce Fields
2011-12-26 18:37 ` Linus Torvalds
0 siblings, 1 reply; 7+ messages in thread
From: J. Bruce Fields @ 2011-12-25 18:19 UTC (permalink / raw)
To: Al Viro; +Cc: Linus Torvalds, linux-nfs, linux-fsdevel, linux-kernel
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: file locking fix for 3.2
2011-12-25 18:19 ` J. Bruce Fields
@ 2011-12-26 18:37 ` Linus Torvalds
2011-12-26 20:18 ` J. Bruce Fields
0 siblings, 1 reply; 7+ messages in thread
From: Linus Torvalds @ 2011-12-26 18:37 UTC (permalink / raw)
To: J. Bruce Fields
Cc: Al Viro, Linus Torvalds, linux-nfs, linux-fsdevel, linux-kernel
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;
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: file locking fix for 3.2
2011-12-26 18:37 ` Linus Torvalds
@ 2011-12-26 20:18 ` J. Bruce Fields
0 siblings, 0 replies; 7+ messages in thread
From: J. Bruce Fields @ 2011-12-26 20:18 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Al Viro, linux-nfs, linux-fsdevel, linux-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-12-26 20:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-24 21:50 file locking fix for 3.2 J. Bruce Fields
2011-12-24 22:55 ` Al Viro
[not found] ` <20111224225525.GR23916-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
2011-12-24 23:50 ` J. Bruce Fields
2011-12-25 0:05 ` Al Viro
2011-12-25 18:19 ` J. Bruce Fields
2011-12-26 18:37 ` Linus Torvalds
2011-12-26 20:18 ` J. Bruce Fields
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).