[PATCH 0/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[PATCH 0/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[Filipe Brandenburger]

Hi,

There seems to be a regression in kernel 2.6.37 regarding the behavior of
fcntl(F_SETLEASE) when called for the second or third time. If fcntl(F_SETOWN)
or fcntl(F_SETSIG) was called before it, the second fcntl(F_SETLEASE) would undo
those settings.

It seems that the regression was introduced by these two commits which were
fixing a leak in a file_lock struct:
* 3df057ac9afe83c4af84016df3baf3a0eb1d3d33
* 8896b93f42459b18b145c69d399b62870df48061

To prevent the leak, a call to locks_free_lock() was added to free the file_lock
struct that was allocated but not used because the old one was reused. The
problem is that the allocated file_lock was already partially initialized with a
pointer to the file descriptor and that locks_free_lock() will then call
locks_release_private() which will call lease_release_private_callback() which
will call f_delown() and set f_owner.signum to 0, effectively undoing the
effects of fcntl(F_SETOWN) and fcntl(F_SETSIG) on that same file descriptor.

I thought of a solution in the lines of having lease_init() set fl_lmops = NULL
and have it set back to point to lease_manager_ops inside do_fcntl_add_lease()
only after it is valid, but I didn't manage to make that work... I also thought
of changing locks_free_lock() somehow to decide whether locks_release_private()
should be called or not (maybe an extra __locks_free_lock() function that would
not call locks_release_private() and could be inlined into locks_free_lock()?)
but in the end I decided to just replace the calls to locks_free_lock(fl) with
kmem_cache_free(filelock_cache, fl) directly on the places where the file_lock
struct was not yet fully initialized.

This issue was found on Samba and reported to their bugzilla at:
https://bugzilla.samba.org/show_bug.cgi?id=8974

A more detailed description and discussion including a test case is at:
https://bugzilla.kernel.org/show_bug.cgi?id=43336

Thanks,
Filipe


Filipe Brandenburger (1):
  locks: prevent side-effects of locks_release_private before file_lock
    is initialized

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

-- 
1.7.7.6

[PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[Filipe Brandenburger]

When calling fcntl(F_SETLEASE) for a second time on the same fd, do_fcntl_add_lease
will allocate and initialize a new file_lock, then if __vfs_setlease decides to
reuse the existing file_lock it will free the newly allocated one to prevent leaking
memory.

However, the new file_lock was initialized to the point where it has a valid file
descriptor pointer and lmops, so calling locks_free_lock will trigger a call to
lease_release_private_callback which will have the side effect of clearing the
fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file descriptor even though
that was not supposed to happen at that point.

This patch will fix this by calling kmem_cache_free(filelock_cache, fl) instead of
locks_free_lock(fl) if the file_lock is not completely initialized and actually
associated to the file descriptor, avoiding the call to lease_release_private_callback
with the undesired side effects.

Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
---
 fs/locks.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 814c51d..ce57c59 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -473,7 +473,7 @@ static struct file_lock *lease_alloc(struct file *filp, int type)
 
 	error = lease_init(filp, type, fl);
 	if (error) {
-		locks_free_lock(fl);
+		kmem_cache_free(filelock_cache, fl);
 		return ERR_PTR(error);
 	}
 	return fl;
@@ -1538,7 +1538,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
 
 	new = fasync_alloc();
 	if (!new) {
-		locks_free_lock(fl);
+		kmem_cache_free(filelock_cache, fl);
 		return -ENOMEM;
 	}
 	ret = fl;
@@ -1546,11 +1546,11 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
 	error = __vfs_setlease(filp, arg, &ret);
 	if (error) {
 		unlock_flocks();
-		locks_free_lock(fl);
+		kmem_cache_free(filelock_cache, fl);
 		goto out_free_fasync;
 	}
 	if (ret != fl)
-		locks_free_lock(fl);
+		kmem_cache_free(filelock_cache, fl);
 
 	/*
 	 * fasync_insert_entry() returns the old entry if any.
-- 
1.7.7.6

Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[J. Bruce Fields]

On Fri, Jun 15, 2012 at 11:06:05PM -0400, Filipe Brandenburger wrote:
> When calling fcntl(F_SETLEASE) for a second time on the same fd, do_fcntl_add_lease
> will allocate and initialize a new file_lock, then if __vfs_setlease decides to
> reuse the existing file_lock it will free the newly allocated one to prevent leaking
> memory.
> 
> However, the new file_lock was initialized to the point where it has a valid file
> descriptor pointer and lmops, so calling locks_free_lock will trigger a call to
> lease_release_private_callback which will have the side effect of clearing the
> fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file descriptor even though
> that was not supposed to happen at that point.
> 
> This patch will fix this by calling kmem_cache_free(filelock_cache, fl) instead of
> locks_free_lock(fl) if the file_lock is not completely initialized and actually
> associated to the file descriptor, avoiding the call to lease_release_private_callback
> with the undesired side effects.

Thanks for catching this!

The result doesn't feel entirely obvious to me.  We could consolidate
the two kmem_cache_free calls and add a comment saying why we're not
calling locks_free_lock().

But clearest might be to separate allocation and initialization and
delay the latter till we know we're going to need it?

--b.

> 
> Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
> ---
>  fs/locks.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 814c51d..ce57c59 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -473,7 +473,7 @@ static struct file_lock *lease_alloc(struct file *filp, int type)
>  
>  	error = lease_init(filp, type, fl);
>  	if (error) {
> -		locks_free_lock(fl);
> +		kmem_cache_free(filelock_cache, fl);
>  		return ERR_PTR(error);
>  	}
>  	return fl;
> @@ -1538,7 +1538,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
>  
>  	new = fasync_alloc();
>  	if (!new) {
> -		locks_free_lock(fl);
> +		kmem_cache_free(filelock_cache, fl);
>  		return -ENOMEM;
>  	}
>  	ret = fl;
> @@ -1546,11 +1546,11 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
>  	error = __vfs_setlease(filp, arg, &ret);
>  	if (error) {
>  		unlock_flocks();
> -		locks_free_lock(fl);
> +		kmem_cache_free(filelock_cache, fl);
>  		goto out_free_fasync;
>  	}
>  	if (ret != fl)
> -		locks_free_lock(fl);
> +		kmem_cache_free(filelock_cache, fl);
>  
>  	/*
>  	 * fasync_insert_entry() returns the old entry if any.
> -- 
> 1.7.7.6
> 

Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[Filipe Brandenburger]

Hi,

On Mon, Jun 18, 2012 at 4:01 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> But clearest might be to separate allocation and initialization and
> delay the latter till we know we're going to need it?

I started playing with this second idea of yours... Unfortunately
having fl_lmops unset before it's fully initialized doesn't really
work because its methods are needed by vfs_setlease()... also, that
would change the API for other users of that function...

But I thought of a way of setting a flag to mark the struct as
uninitialized and then have vfs_setlease() clear that flag once it
decides to use that struct. If it doesn't and changes the flp pointer
to the one that was in use before, then it doesn't clear that flag
which means the locks_free_lock() function will not do any calls into
fl_lmops which might have side effects on the process...

Here's the patch:


diff --git a/include/linux/fs.h b/include/linux/fs.h
index 17fd887..8da1217 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1129,6 +1129,7 @@ static inline int file_check_writeable(struct file *filp)
 #define FL_SLEEP       128     /* A blocking lock */
 #define FL_DOWNGRADE_PENDING   256 /* Lease is being downgraded */
 #define FL_UNLOCK_PENDING      512 /* Lease is being broken */
+#define FL_LEASE_UNINITIALIZED 1024 /* Lease was not fully initialized yet */

 /*
  * Special return value from posix_lock_file() and vfs_lock_file() for
diff --git a/fs/locks.c b/fs/locks.c
index 814c51d..a995cc9 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -195,6 +195,8 @@ EXPORT_SYMBOL_GPL(locks_alloc_lock);

 void locks_release_private(struct file_lock *fl)
 {
+       if (fl->fl_flags & FL_LEASE_UNINITIALIZED)
+               return;
        if (fl->fl_ops) {
                if (fl->fl_ops->fl_release_private)
                        fl->fl_ops->fl_release_private(fl);
@@ -454,7 +456,7 @@ static int lease_init(struct file *filp, int type,
struct file_lock *fl)
        fl->fl_pid = current->tgid;

        fl->fl_file = filp;
-       fl->fl_flags = FL_LEASE;
+       fl->fl_flags = FL_LEASE | FL_LEASE_UNINITIALIZED;
        fl->fl_start = 0;
        fl->fl_end = OFFSET_MAX;
        fl->fl_ops = NULL;
@@ -1406,6 +1408,7 @@ int generic_add_lease(struct file *filp, long
arg, struct file_lock **flp)
        if (!leases_enable)
                goto out;

+       lease->fl_flags &= ~FL_LEASE_UNINITIALIZED;
        locks_insert_lock(before, lease);
        return 0;



Unfortunately, at this point I have more questions than answers...
* Is this a good idea?
* Is it good to store this as an extra flag? Or would it be
preferrable to add an extra boolean field to the file_lock struct
instead?
* Is FL_LEASE_UNINITIALIZED a good name for this flag?
* Should this flag be set only in lease_init()? Or also functions such
as flock_make_lock()? Potentially everything that is freed with
locks_free_lock() might need it...
* Should the flag be cleared in generic_add_lease() only? Or should
__vfs_setlease() compare the original value of the lease pointer with
the one returned by generic_setlease() (or filp->f_op->setlease() if
it's set) and then clear the flag itself?

Also, looking at lease_alloc(), I noticed that it calls
locks_free_lock() if lease_init() fails, but lease_init() can only
fail right at the beginning if assign_type() fails, but at that point
can it be guaranteed that fl_lmops (and fl_ops for that matter) are
properly populated or are at least NULL? Maybe locks_alloc_lock()
should initialize the file_lock struct with a flag indicating that
it's not fully initialized at that point yet...

Another thing I noticed (after Bruce pointed me to that code) is that
fs/nfsd/nfs4state.c:nfs4_setlease() seems to suffer from the leak bug
that was fixed by the commits which introduced this issue, it's not
checking whether vfs_setlease() is returning a different file_lock
struct than the one it allocated and in that case it's not freeing the
one it passed. But I'd say it's probably best to figure out a fix for
that one only after this one is fixed to avoid introducing the
regression there as well.

Cheers,
Filipe

Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[J. Bruce Fields]

On Tue, Jun 19, 2012 at 10:39:55PM -0400, Filipe Brandenburger wrote:
> Hi,
> 
> On Mon, Jun 18, 2012 at 4:01 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > But clearest might be to separate allocation and initialization and
> > delay the latter till we know we're going to need it?
> 
> I started playing with this second idea of yours... Unfortunately
> having fl_lmops unset before it's fully initialized doesn't really
> work because its methods are needed by vfs_setlease()... also, that
> would change the API for other users of that function...
> 
> But I thought of a way of setting a flag to mark the struct as
> uninitialized and then have vfs_setlease() clear that flag once it
> decides to use that struct. If it doesn't and changes the flp pointer
> to the one that was in use before, then it doesn't clear that flag
> which means the locks_free_lock() function will not do any calls into
> fl_lmops which might have side effects on the process...
> 
> Here's the patch:
> 
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 17fd887..8da1217 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1129,6 +1129,7 @@ static inline int file_check_writeable(struct file *filp)
>  #define FL_SLEEP       128     /* A blocking lock */
>  #define FL_DOWNGRADE_PENDING   256 /* Lease is being downgraded */
>  #define FL_UNLOCK_PENDING      512 /* Lease is being broken */
> +#define FL_LEASE_UNINITIALIZED 1024 /* Lease was not fully initialized yet */
> 
>  /*
>   * Special return value from posix_lock_file() and vfs_lock_file() for
> diff --git a/fs/locks.c b/fs/locks.c
> index 814c51d..a995cc9 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -195,6 +195,8 @@ EXPORT_SYMBOL_GPL(locks_alloc_lock);
> 
>  void locks_release_private(struct file_lock *fl)
>  {
> +       if (fl->fl_flags & FL_LEASE_UNINITIALIZED)
> +               return;

I don't know, it bugs me a little to muck up common lock code with
an odd exception for the lease code.

Let's just go with your first patch and free the thing by hand (but add
a comment explaining why).

Then come back and figure out how to make the interface clearer once
we've got the bug fixed.

>         if (fl->fl_ops) {
>                 if (fl->fl_ops->fl_release_private)
>                         fl->fl_ops->fl_release_private(fl);
> @@ -454,7 +456,7 @@ static int lease_init(struct file *filp, int type,
> struct file_lock *fl)
>         fl->fl_pid = current->tgid;
> 
>         fl->fl_file = filp;
> -       fl->fl_flags = FL_LEASE;
> +       fl->fl_flags = FL_LEASE | FL_LEASE_UNINITIALIZED;
>         fl->fl_start = 0;
>         fl->fl_end = OFFSET_MAX;
>         fl->fl_ops = NULL;
> @@ -1406,6 +1408,7 @@ int generic_add_lease(struct file *filp, long
> arg, struct file_lock **flp)
>         if (!leases_enable)
>                 goto out;
> 
> +       lease->fl_flags &= ~FL_LEASE_UNINITIALIZED;
>         locks_insert_lock(before, lease);
>         return 0;
> 
> 
> 
> Unfortunately, at this point I have more questions than answers...
> * Is this a good idea?
> * Is it good to store this as an extra flag? Or would it be
> preferrable to add an extra boolean field to the file_lock struct
> instead?
> * Is FL_LEASE_UNINITIALIZED a good name for this flag?
> * Should this flag be set only in lease_init()? Or also functions such
> as flock_make_lock()? Potentially everything that is freed with
> locks_free_lock() might need it...
> * Should the flag be cleared in generic_add_lease() only? Or should
> __vfs_setlease() compare the original value of the lease pointer with
> the one returned by generic_setlease() (or filp->f_op->setlease() if
> it's set) and then clear the flag itself?
> 
> Also, looking at lease_alloc(), I noticed that it calls
> locks_free_lock() if lease_init() fails, but lease_init() can only
> fail right at the beginning if assign_type() fails, but at that point
> can it be guaranteed that fl_lmops (and fl_ops for that matter) are
> properly populated or are at least NULL? Maybe locks_alloc_lock()
> should initialize the file_lock struct with a flag indicating that
> it's not fully initialized at that point yet...

locks_alloc_lock() uses kmem_cache_zalloc(), so this is ok--the memory
is zeroed.

> Another thing I noticed (after Bruce pointed me to that code) is that
> fs/nfsd/nfs4state.c:nfs4_setlease() seems to suffer from the leak bug
> that was fixed by the commits which introduced this issue, it's not
> checking whether vfs_setlease() is returning a different file_lock
> struct than the one it allocated and in that case it's not freeing the
> one it passed.

The v4 code shouldn't ever hit the case where two leases are "merged",
but that should be made more obvious.

--b.

> But I'd say it's probably best to figure out a fix for
> that one only after this one is fixed to avoid introducing the
> regression there as well.
> 
> Cheers,
> Filipe

Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[Filipe Brandenburger]

Hi Bruce,

I was just reviewing this set of patches today... I think if the idea
is not to call fl->fl_lmops->lm_release_private(fl) when the file_lock
struct was not used, then I'd prefer to introduce an exported
__locks_free_lock() function that would do it in order not to expose
the kmem_cache implementation and allow other implementations to do
it.

But I was reading the code and thinking a little more about it and now
I think the correct behavior should be to always call
fl->fl_lmops->lm_release_private(fl) (if the pointers are not NULL)
and have that function behave appropriately if the file_lock struct
was not used.

What made me think of that was a use of fl_ops (it's not directly
fl_lmops, but still I think it would be nice to keep a similar
interface) where nfs4_fl_lock_ops.fl_release_private =
nfs4_fl_release_lock and nfs4_fl_release_lock calls
nfs4_put_lock_state which frees some lists and decrements the usage
counter... Not calling fl->fl_ops->fl_release_private(fl) in that
particular case would be clearly wrong...

So I was thinking of tracking whether the lease was assigned, probably
setting the flag in vfs_setlease(), and then changing
lease_release_private_callback() to check whether the flag was set and
only resetting the F_SETOWN and F_SETSIG information if the flag was
set...

What do you think of that idea?

I just got a quick diff which outlines what I'm thinking of, I haven't
tested it yet, I'll try to build it and run it to see if it passes the
testcase. But please let me know what you think of it.

Cheers,
Fil

------- >8 cut here -------


diff --git a/fs/locks.c b/fs/locks.c
index 814c51d..242ac84 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -429,7 +429,7 @@ static void lease_break_callback(struct file_lock *fl)

 static void lease_release_private_callback(struct file_lock *fl)
 {
-       if (!fl->fl_file)
+       if (!fl->fl_file || !fl->fl_lease_inuse)
                return;

        f_delown(fl->fl_file);
@@ -1513,6 +1513,9 @@ int vfs_setlease(struct file *filp, long arg,
struct file_lock **lease)
        error = __vfs_setlease(filp, arg, lease);
        unlock_flocks();

+       if (!error)
+               lease->fl_lease_inuse = 1;
+
        return error;
 }
 EXPORT_SYMBOL_GPL(vfs_setlease);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 17fd887..2c577a9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1176,6 +1176,7 @@ struct file_lock {
        struct list_head fl_block;      /* circular list of blocked processes */
        fl_owner_t fl_owner;
        unsigned int fl_flags;
+       unsigned char fl_lease_inuse;
        unsigned char fl_type;
        unsigned int fl_pid;
        struct pid *fl_nspid;

Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[Filipe Brandenburger]

Hi Bruce,

Just to let you know that I just tested the patch below on top of
3.5.0-rc4 and it works fine...

Do you like the idea of this second patch or do you prefer the
__locks_free_lock() one?

Do you agree with the name "fl_lease_inuse" for the field in file_lock
struct to track whether the lease was initialized/assigned?

May I go ahead and submit a PATCHv2 for this fix?

Cheers,
Filipe


On Mon, Jun 25, 2012 at 8:48 PM, Filipe Brandenburger
<filbranden@gmail.com> wrote:
> Hi Bruce,
>
> I was just reviewing this set of patches today... I think if the idea
> is not to call fl->fl_lmops->lm_release_private(fl) when the file_lock
> struct was not used, then I'd prefer to introduce an exported
> __locks_free_lock() function that would do it in order not to expose
> the kmem_cache implementation and allow other implementations to do
> it.
>
> But I was reading the code and thinking a little more about it and now
> I think the correct behavior should be to always call
> fl->fl_lmops->lm_release_private(fl) (if the pointers are not NULL)
> and have that function behave appropriately if the file_lock struct
> was not used.
>
> What made me think of that was a use of fl_ops (it's not directly
> fl_lmops, but still I think it would be nice to keep a similar
> interface) where nfs4_fl_lock_ops.fl_release_private =
> nfs4_fl_release_lock and nfs4_fl_release_lock calls
> nfs4_put_lock_state which frees some lists and decrements the usage
> counter... Not calling fl->fl_ops->fl_release_private(fl) in that
> particular case would be clearly wrong...
>
> So I was thinking of tracking whether the lease was assigned, probably
> setting the flag in vfs_setlease(), and then changing
> lease_release_private_callback() to check whether the flag was set and
> only resetting the F_SETOWN and F_SETSIG information if the flag was
> set...
>
> What do you think of that idea?
>
> I just got a quick diff which outlines what I'm thinking of, I haven't
> tested it yet, I'll try to build it and run it to see if it passes the
> testcase. But please let me know what you think of it.
>
> Cheers,
> Fil
>
> ------- >8 cut here -------
>
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 814c51d..242ac84 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -429,7 +429,7 @@ static void lease_break_callback(struct file_lock *fl)
>
>  static void lease_release_private_callback(struct file_lock *fl)
>  {
> -       if (!fl->fl_file)
> +       if (!fl->fl_file || !fl->fl_lease_inuse)
>                return;
>
>        f_delown(fl->fl_file);
> @@ -1513,6 +1513,9 @@ int vfs_setlease(struct file *filp, long arg,
> struct file_lock **lease)
>        error = __vfs_setlease(filp, arg, lease);
>        unlock_flocks();
>
> +       if (!error)
> +               lease->fl_lease_inuse = 1;
> +
>        return error;
>  }
>  EXPORT_SYMBOL_GPL(vfs_setlease);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 17fd887..2c577a9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1176,6 +1176,7 @@ struct file_lock {
>        struct list_head fl_block;      /* circular list of blocked processes */
>        fl_owner_t fl_owner;
>        unsigned int fl_flags;
> +       unsigned char fl_lease_inuse;
>        unsigned char fl_type;
>        unsigned int fl_pid;
>        struct pid *fl_nspid;

Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[J. Bruce Fields]

On Mon, Jun 25, 2012 at 10:10:35PM -0400, Filipe Brandenburger wrote:
> Just to let you know that I just tested the patch below on top of
> 3.5.0-rc4 and it works fine...
> 
> Do you like the idea of this second patch or do you prefer the
> __locks_free_lock() one?

Like I said:

> Let's just go with your first patch and free the thing by hand (but
> add a comment explaining why).
> 
> Then come back and figure out how to make the interface clearer once
> we've got the bug fixed.

--b.

[PATCH v2 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[Filipe Brandenburger]

When calling fcntl(F_SETLEASE) for a second time on the same file descriptor,
do_fcntl_add_lease will allocate and initialize a new file_lock to pass to
__vfs_setlease. If that function decides to reuse the existing file_lock, it
will free the newly allocated one to prevent leaking memory.

However, the newly allocate file_lock was initialized with a valid file
descriptor pointer and fl_lmops that contains a lm_release_private function,
so calling locks_free_lock will trigger a call to lease_release_private_callback
which will clear the fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file
descriptor even though the lease is not really being cleared at that point (as
only the temporary file_lock is being freed.)

This patch will fix this bug by calling kmem_cache_free directly instead of
locks_free_lock if the file_lock will not be used. This will end up avoiding
the call to lease_release_private_callback.

This bug was tracked in kernel.org Bugzilla database:
https://bugzilla.kernel.org/show_bug.cgi?id=43336

Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
---
 fs/locks.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 814c51d..2a751d8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -473,7 +473,10 @@ static struct file_lock *lease_alloc(struct file *filp, int type)
 
 	error = lease_init(filp, type, fl);
 	if (error) {
-		locks_free_lock(fl);
+		/* Free the lock by hand instead of calling locks_free_lock
+		 * to prevent the call back to lease_release_private_callback
+		 */
+		kmem_cache_free(filelock_cache, fl);
 		return ERR_PTR(error);
 	}
 	return fl;
@@ -1538,19 +1541,28 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
 
 	new = fasync_alloc();
 	if (!new) {
-		locks_free_lock(fl);
+		/* Free the lock by hand instead of calling locks_free_lock
+		 * to prevent the call back to lease_release_private_callback
+		 */
+		kmem_cache_free(filelock_cache, fl);
 		return -ENOMEM;
 	}
 	ret = fl;
 	lock_flocks();
 	error = __vfs_setlease(filp, arg, &ret);
+	if (error || ret != fl)
+		/*
+		 * Free the lock by hand instead of calling locks_free_lock
+		 * to prevent the call back to lease_release_private_callback
+		 * which will unset F_SETOWN and F_SETSIG for the file
+		 * descriptor but that is not wanted as the lease was not
+		 * really in use.
+		 */
+		kmem_cache_free(filelock_cache, fl);
 	if (error) {
 		unlock_flocks();
-		locks_free_lock(fl);
 		goto out_free_fasync;
 	}
-	if (ret != fl)
-		locks_free_lock(fl);
 
 	/*
 	 * fasync_insert_entry() returns the old entry if any.
-- 
1.7.7.6

Re: [PATCH v2 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[J. Bruce Fields]

On Tue, Jun 26, 2012 at 09:50:48PM -0400, Filipe Brandenburger wrote:
> When calling fcntl(F_SETLEASE) for a second time on the same file descriptor,
> do_fcntl_add_lease will allocate and initialize a new file_lock to pass to
> __vfs_setlease. If that function decides to reuse the existing file_lock, it
> will free the newly allocated one to prevent leaking memory.
> 
> However, the newly allocate file_lock was initialized with a valid file
> descriptor pointer and fl_lmops that contains a lm_release_private function,
> so calling locks_free_lock will trigger a call to lease_release_private_callback
> which will clear the fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file
> descriptor even though the lease is not really being cleared at that point (as
> only the temporary file_lock is being freed.)
> 
> This patch will fix this bug by calling kmem_cache_free directly instead of
> locks_free_lock if the file_lock will not be used. This will end up avoiding
> the call to lease_release_private_callback.
> 
> This bug was tracked in kernel.org Bugzilla database:
> https://bugzilla.kernel.org/show_bug.cgi?id=43336
> 
> Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
> ---
>  fs/locks.c |   22 +++++++++++++++++-----
>  1 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 814c51d..2a751d8 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -473,7 +473,10 @@ static struct file_lock *lease_alloc(struct file *filp, int type)
>  
>  	error = lease_init(filp, type, fl);
>  	if (error) {
> -		locks_free_lock(fl);
> +		/* Free the lock by hand instead of calling locks_free_lock
> +		 * to prevent the call back to lease_release_private_callback
> +		 */
> +		kmem_cache_free(filelock_cache, fl);
>  		return ERR_PTR(error);
>  	}
>  	return fl;
> @@ -1538,19 +1541,28 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
>  
>  	new = fasync_alloc();
>  	if (!new) {
> -		locks_free_lock(fl);
> +		/* Free the lock by hand instead of calling locks_free_lock
> +		 * to prevent the call back to lease_release_private_callback
> +		 */
> +		kmem_cache_free(filelock_cache, fl);
>  		return -ENOMEM;
>  	}
>  	ret = fl;
>  	lock_flocks();
>  	error = __vfs_setlease(filp, arg, &ret);
> +	if (error || ret != fl)
> +		/*
> +		 * Free the lock by hand instead of calling locks_free_lock
> +		 * to prevent the call back to lease_release_private_callback
> +		 * which will unset F_SETOWN and F_SETSIG for the file
> +		 * descriptor but that is not wanted as the lease was not
> +		 * really in use.
> +		 */
> +		kmem_cache_free(filelock_cache, fl);

Ugh, and now we end up with essentially the same comment in 3 different
places.  So maybe marking the lock somehow *was* the better idea.

--b.

>  	if (error) {
>  		unlock_flocks();
> -		locks_free_lock(fl);
>  		goto out_free_fasync;
>  	}
> -	if (ret != fl)
> -		locks_free_lock(fl);
>  
>  	/*
>  	 * fasync_insert_entry() returns the old entry if any.
> -- 
> 1.7.7.6
> 

Re: [PATCH v2 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized

[J. Bruce Fields]

On Thu, Jul 05, 2012 at 06:42:51PM -0400, J. Bruce Fields wrote:
> On Tue, Jun 26, 2012 at 09:50:48PM -0400, Filipe Brandenburger wrote:
> > When calling fcntl(F_SETLEASE) for a second time on the same file descriptor,
> > do_fcntl_add_lease will allocate and initialize a new file_lock to pass to
> > __vfs_setlease. If that function decides to reuse the existing file_lock, it
> > will free the newly allocated one to prevent leaking memory.
> > 
> > However, the newly allocate file_lock was initialized with a valid file
> > descriptor pointer and fl_lmops that contains a lm_release_private function,
> > so calling locks_free_lock will trigger a call to lease_release_private_callback
> > which will clear the fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file
> > descriptor even though the lease is not really being cleared at that point (as
> > only the temporary file_lock is being freed.)
> > 
> > This patch will fix this bug by calling kmem_cache_free directly instead of
> > locks_free_lock if the file_lock will not be used. This will end up avoiding
> > the call to lease_release_private_callback.
> > 
> > This bug was tracked in kernel.org Bugzilla database:
> > https://bugzilla.kernel.org/show_bug.cgi?id=43336

Apologies for not taking a closer look till now.

Having done so.... I think the real problem is that we have this
release callback at all.  The fact is, this doesn't really have anything
to do with releasing resources.  And of the places we free a lease, I
believe only one of them actually wants this.

That callback was added without any visible justification by the patch
below, pulled out of one of the historical git repos.  It looks to me
like we could just revert it.

Could you try that?  If it works, could you send in the result with
proper changelog, etc.?

--b.

commit fd08d90925ac99d076382bcf4089085f92992954
Author: William A. Adamson <andros@thnk.citi.umich.edu>
Date:   Tue Oct 19 18:44:22 2004 -0700

    [PATCH] nfs4 lease: move the f_delown processing
    
    Move the f_delown processing from lease_modify() into a new default lock
    manager fl_release_private callback.
    
    Signed-off-by: Andy Adamson <andros@citi.umich.edu>
    Signed-off-by: Andrew Morton <akpm@osdl.org>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/fs/locks.c b/fs/locks.c
index e05dffc..1257539 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -397,8 +397,18 @@ static void lease_break_callback(struct file_lock *fl)
 	kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG);
 }
 
+static void lease_release_private_callback(struct file_lock *fl)
+{
+	if (!fl->fl_file)
+		return;
+
+	f_delown(fl->fl_file);
+	fl->fl_file->f_owner.signum = 0;
+}
+
 struct lock_manager_operations lease_manager_ops = {
 	.fl_break = lease_break_callback,
+	.fl_release_private = lease_release_private_callback,
 };
 
 /*
@@ -1056,13 +1066,8 @@ static int lease_modify(struct file_lock **before, int arg)
 	if (error)
 		return error;
 	locks_wake_up_blocks(fl);
-	if (arg == F_UNLCK) {
-		struct file *filp = fl->fl_file;
-
-		f_delown(filp);
-		filp->f_owner.signum = 0;
+	if (arg == F_UNLCK)
 		locks_delete_lock(before);
-	}
 	return 0;
 }
 

[PATCHv3] locks: prevent side-effects of locks_release_private before file_lock is initialized

[Filipe Brandenburger]

When calling fcntl(fd, F_SETLEASE, lck) [with lck=F_WRLCK or F_RDLCK],
the custom signal or owner (if any were previously set using F_SETSIG
or F_SETOWN fcntls) would be reset when F_SETLEASE was called for the
second time on the same file descriptor.

This bug is a regression of 2.6.37 and is described here:
https://bugzilla.kernel.org/show_bug.cgi?id=43336

This patch reverts a commit from Oct 2004 (with subject "nfs4 lease:
move the f_delown processing") which originally introduced the
lm_release_private callback.

Signed-off-by: Filipe Brandenburger <filbranden@gmail.com>
---
 fs/locks.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 82c3533..6595882 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -427,18 +427,8 @@ static void lease_break_callback(struct file_lock *fl)
 	kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG);
 }
 
-static void lease_release_private_callback(struct file_lock *fl)
-{
-	if (!fl->fl_file)
-		return;
-
-	f_delown(fl->fl_file);
-	fl->fl_file->f_owner.signum = 0;
-}
-
 static const struct lock_manager_operations lease_manager_ops = {
 	.lm_break = lease_break_callback,
-	.lm_release_private = lease_release_private_callback,
 	.lm_change = lease_modify,
 };
 
@@ -1155,8 +1145,13 @@ int lease_modify(struct file_lock **before, int arg)
 		return error;
 	lease_clear_pending(fl, arg);
 	locks_wake_up_blocks(fl);
-	if (arg == F_UNLCK)
+	if (arg == F_UNLCK) {
+		struct file *filp = fl->fl_file;
+
+		f_delown(filp);
+		filp->f_owner.signum = 0;
 		locks_delete_lock(before);
+	}
 	return 0;
 }
 
-- 
1.7.7.6

Re: [PATCHv3] locks: prevent side-effects of locks_release_private before file_lock is initialized

[J. Bruce Fields]

On Fri, Jul 27, 2012 at 12:42:52AM -0400, Filipe Brandenburger wrote:
> When calling fcntl(fd, F_SETLEASE, lck) [with lck=F_WRLCK or F_RDLCK],
> the custom signal or owner (if any were previously set using F_SETSIG
> or F_SETOWN fcntls) would be reset when F_SETLEASE was called for the
> second time on the same file descriptor.
> 
> This bug is a regression of 2.6.37 and is described here:
> https://bugzilla.kernel.org/show_bug.cgi?id=43336
> 
> This patch reverts a commit from Oct 2004 (with subject "nfs4 lease:
> move the f_delown processing") which originally introduced the
> lm_release_private callback.

Looks fine, thanks.  I think can also do something like the following
(on top of your patch).

--b.

commit 96d6d59ceaeaacba4088862f3c57fcd011f52832
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Jul 27 16:18:00 2012 -0400

    locks: move lease-specific code out of locks_delete_lock
    
    No point putting something only used by one caller into common code.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/locks.c b/fs/locks.c
index 86668dd..541075a 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -570,12 +570,6 @@ static void locks_delete_lock(struct file_lock **thisfl_p)
 	fl->fl_next = NULL;
 	list_del_init(&fl->fl_link);
 
-	fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
-	if (fl->fl_fasync != NULL) {
-		printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
-		fl->fl_fasync = NULL;
-	}
-
 	if (fl->fl_nspid) {
 		put_pid(fl->fl_nspid);
 		fl->fl_nspid = NULL;
@@ -1150,6 +1144,11 @@ int lease_modify(struct file_lock **before, int arg)
 
 		f_delown(filp);
 		filp->f_owner.signum = 0;
+		fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
+		if (fl->fl_fasync != NULL) {
+			printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
+			fl->fl_fasync = NULL;
+		}
 		locks_delete_lock(before);
 	}
 	return 0;

Re: [PATCHv3] locks: prevent side-effects of locks_release_private before file_lock is initialized

[J. Bruce Fields]

On Fri, Jul 27, 2012 at 04:45:52PM -0400, J. Bruce Fields wrote:
> On Fri, Jul 27, 2012 at 12:42:52AM -0400, Filipe Brandenburger wrote:
> > When calling fcntl(fd, F_SETLEASE, lck) [with lck=F_WRLCK or F_RDLCK],
> > the custom signal or owner (if any were previously set using F_SETSIG
> > or F_SETOWN fcntls) would be reset when F_SETLEASE was called for the
> > second time on the same file descriptor.
> > 
> > This bug is a regression of 2.6.37 and is described here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=43336
> > 
> > This patch reverts a commit from Oct 2004 (with subject "nfs4 lease:
> > move the f_delown processing") which originally introduced the
> > lm_release_private callback.
> 
> Looks fine, thanks.  I think can also do something like the following
> (on top of your patch).

(Committing this as well.)--b.

> 
> --b.
> 
> commit 96d6d59ceaeaacba4088862f3c57fcd011f52832
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Fri Jul 27 16:18:00 2012 -0400
> 
>     locks: move lease-specific code out of locks_delete_lock
>     
>     No point putting something only used by one caller into common code.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 86668dd..541075a 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -570,12 +570,6 @@ static void locks_delete_lock(struct file_lock **thisfl_p)
>  	fl->fl_next = NULL;
>  	list_del_init(&fl->fl_link);
>  
> -	fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
> -	if (fl->fl_fasync != NULL) {
> -		printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
> -		fl->fl_fasync = NULL;
> -	}
> -
>  	if (fl->fl_nspid) {
>  		put_pid(fl->fl_nspid);
>  		fl->fl_nspid = NULL;
> @@ -1150,6 +1144,11 @@ int lease_modify(struct file_lock **before, int arg)
>  
>  		f_delown(filp);
>  		filp->f_owner.signum = 0;
> +		fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
> +		if (fl->fl_fasync != NULL) {
> +			printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
> +			fl->fl_fasync = NULL;
> +		}
>  		locks_delete_lock(before);
>  	}
>  	return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html