Re: [PATCH 1/1] locks: prevent side-effects of locks_release_privatebefore 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@xxxxxxxxx> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/