lkml.org 
[lkml]   [2012]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized
    From
    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;
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-06-26 04:41    [W:0.030 / U:239.876 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site