[BUGFIX][PATCH 0/3] configfs: symlink() fixes - Kernel

This is a discussion on [BUGFIX][PATCH 0/3] configfs: symlink() fixes - Kernel ; [ applies on top of the previously submitted rename() vs rmdir() deadlock fix ] Hi, The following patchset fixes incorrect symlinks to dead items in configfs, which are forbidden by specification. The first patch actually prevents such dangling symlinks from ...

+ Reply to Thread
Results 1 to 15 of 15

Thread: [BUGFIX][PATCH 0/3] configfs: symlink() fixes

  1. [BUGFIX][PATCH 0/3] configfs: symlink() fixes

    [ applies on top of the previously submitted rename() vs rmdir() deadlock fix ]

    Hi,

    The following patchset fixes incorrect symlinks to dead items in configfs, which
    are forbidden by specification.

    The first patch actually prevents such dangling symlinks from being created, but
    introduces a weird(?) behavior where a failing symlink() can make a racing
    rmdir() fail in the symlink's parent and in the symlink's target as well. The
    next patches prevent this behavior using a similar idea as for the mkdir() vs
    rmdir() case previously submitted.

    Summary:
    configfs: Fix symlink() to a removing item
    configfs: Rename CONFIGFS_USET_IN_MKDIR to CONFIGFS_USET_ATTACHING
    configfs: Fix failing symlink() making rmdir() fail

    fs/configfs/configfs_internal.h | 2 +-
    fs/configfs/dir.c | 20 ++++++++++----------
    fs/configfs/symlink.c | 33 +++++++++++++++++++++++++++++----
    3 files changed, 40 insertions(+), 15 deletions(-)

    --
    Dr Louis Rilling Kerlabs
    Skype: louis.rilling Batiment Germanium
    Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
    http://www.kerlabs.com/ 35700 Rennes
    --
    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/

  2. [BUGFIX][PATCH 2/3] configfs: Rename CONFIGFS_USET_IN_MKDIR to CONFIGFS_USET_ATTACHING

    The CONFIGFS_USET_IN_MKDIR flag can be reused with symlink() to solve a similar
    issue as mkdir() vs rmdir(). This patch renames the flag to make it more
    meaningful for this purpose.

    Signed-off-by: Louis Rilling
    ---
    fs/configfs/configfs_internal.h | 2 +-
    fs/configfs/dir.c | 6 +++---
    2 files changed, 4 insertions(+), 4 deletions(-)

    diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
    index da015c1..a28f37d 100644
    --- a/fs/configfs/configfs_internal.h
    +++ b/fs/configfs/configfs_internal.h
    @@ -48,7 +48,7 @@ struct configfs_dirent {
    #define CONFIGFS_USET_DIR 0x0040
    #define CONFIGFS_USET_DEFAULT 0x0080
    #define CONFIGFS_USET_DROPPING 0x0100
    -#define CONFIGFS_USET_IN_MKDIR 0x0200
    +#define CONFIGFS_USET_ATTACHING 0x0200
    #define CONFIGFS_NOT_PINNED (CONFIGFS_ITEM_ATTR)

    extern spinlock_t configfs_dirent_lock;
    diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
    index f2a12d0..629c938 100644
    --- a/fs/configfs/dir.c
    +++ b/fs/configfs/dir.c
    @@ -383,7 +383,7 @@ static int configfs_detach_prep(struct dentry *dentry, struct mutex **wait_mutex
    continue;
    if (sd->s_type & CONFIGFS_USET_DEFAULT) {
    /* Abort if racing with mkdir() */
    - if (sd->s_type & CONFIGFS_USET_IN_MKDIR) {
    + if (sd->s_type & CONFIGFS_USET_ATTACHING) {
    if (wait_mutex)
    *wait_mutex = &sd->s_dentry->d_inode->i_mutex;
    return -EAGAIN;
    @@ -1127,7 +1127,7 @@ static int configfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
    */
    spin_lock(&configfs_dirent_lock);
    /* This will make configfs_detach_prep() fail */
    - sd->s_type |= CONFIGFS_USET_IN_MKDIR;
    + sd->s_type |= CONFIGFS_USET_ATTACHING;
    spin_unlock(&configfs_dirent_lock);

    if (group)
    @@ -1136,7 +1136,7 @@ static int configfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
    ret = configfs_attach_item(parent_item, item, dentry);

    spin_lock(&configfs_dirent_lock);
    - sd->s_type &= ~CONFIGFS_USET_IN_MKDIR;
    + sd->s_type &= ~CONFIGFS_USET_ATTACHING;
    spin_unlock(&configfs_dirent_lock);

    out_unlink:
    --
    1.5.5.3

    --
    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/

  3. [BUGFIX][PATCH 1/3] configfs: Fix symlink() to a removing item

    The rule for configfs symlinks is that symlinks always point to valid
    config_items, and prevent the target from being removed. However,
    configfs_symlink() only checks that it can grab a reference on the target item,
    without ensuring that it remains alive until the symlink is correctly attached.

    This patch makes configfs_symlink() fail whenever the target is being removed,
    using the CONFIGFS_USET_DROPPING flag set by configfs_detach_prep() and
    protected by configfs_dirent_lock.

    This patch introduces a similar (weird?) behavior as with mkdir failures making
    rmdir fail: if symlink() races with rmdir() of the parent directory (or its
    youngest user-created ancestor if parent is a default group) or rmdir() of the
    target directory, and then fails in configfs_create(), this can make the racing
    rmdir() fail despite the concerned directory having no user-created entry (resp.
    no symlink pointing to it or one of its default groups) in the end.
    This behavior is fixed in later patches.

    Signed-off-by: Louis Rilling
    ---
    fs/configfs/dir.c | 14 +++++++-------
    fs/configfs/symlink.c | 6 ++++++
    2 files changed, 13 insertions(+), 7 deletions(-)

    diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
    index 614e382..f2a12d0 100644
    --- a/fs/configfs/dir.c
    +++ b/fs/configfs/dir.c
    @@ -370,6 +370,9 @@ static int configfs_detach_prep(struct dentry *dentry, struct mutex **wait_mutex
    struct configfs_dirent *sd;
    int ret;

    + /* Mark that we're trying to drop the group */
    + parent_sd->s_type |= CONFIGFS_USET_DROPPING;
    +
    ret = -EBUSY;
    if (!list_empty(&parent_sd->s_links))
    goto out;
    @@ -385,8 +388,6 @@ static int configfs_detach_prep(struct dentry *dentry, struct mutex **wait_mutex
    *wait_mutex = &sd->s_dentry->d_inode->i_mutex;
    return -EAGAIN;
    }
    - /* Mark that we're trying to drop the group */
    - sd->s_type |= CONFIGFS_USET_DROPPING;

    /*
    * Yup, recursive. If there's a problem, blame
    @@ -414,12 +415,11 @@ static void configfs_detach_rollback(struct dentry *dentry)
    struct configfs_dirent *parent_sd = dentry->d_fsdata;
    struct configfs_dirent *sd;

    - list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
    - if (sd->s_type & CONFIGFS_USET_DEFAULT) {
    + parent_sd->s_type &= ~CONFIGFS_USET_DROPPING;
    +
    + list_for_each_entry(sd, &parent_sd->s_children, s_sibling)
    + if (sd->s_type & CONFIGFS_USET_DEFAULT)
    configfs_detach_rollback(sd->s_dentry);
    - sd->s_type &= ~CONFIGFS_USET_DROPPING;
    - }
    - }
    }

    static void detach_attrs(struct config_item * item)
    diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
    index faeb441..58722a9 100644
    --- a/fs/configfs/symlink.c
    +++ b/fs/configfs/symlink.c
    @@ -78,6 +78,12 @@ static int create_link(struct config_item *parent_item,
    if (sl) {
    sl->sl_target = config_item_get(item);
    spin_lock(&configfs_dirent_lock);
    + if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
    + spin_unlock(&configfs_dirent_lock);
    + config_item_put(item);
    + kfree(sl);
    + return -EPERM;
    + }
    list_add(&sl->sl_list, &target_sd->s_links);
    spin_unlock(&configfs_dirent_lock);
    ret = configfs_create_link(sl, parent_item->ci_dentry,
    --
    1.5.5.3

    --
    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/

  4. [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

    On a similar pattern as mkdir() vs rmdir(), a failing symlink() may make rmdir()
    fail for the symlink's parent and the symlink's target as well.

    failing symlink() making target's rmdir() fail:

    process 1: process 2:
    symlink("A/S" -> "B")
    allow_link()
    create_link()
    attach to "B" links list
    rmdir("B")
    detach_prep("B")
    error because of new link
    configfs_create_link("A", "S")
    error (eg -ENOMEM)

    failing symlink() making parent's rmdir() fail:

    process 1: process 2:
    symlink("A/D/S" -> "B")
    allow_link()
    create_link()
    attach to "B" links list
    configfs_create_link("A/D", "S")
    make_dirent("A/D", "S")
    rmdir("A")
    detach_prep("A")
    detach_prep("A/D")
    error because of "S"
    create("S")
    error (eg -ENOMEM)

    For the parent's rmdir() case, we can use the same solution as with mkdir() vs
    rmdir(). For the target's rmdir() case, we cannot, since we do not and cannot
    lock the target's inode while in symlink(). Fortunately, once create_link()
    terminates, no further operation can fail in symlink(). So we first reorder the
    operations in create_link() to attach the new symlink to its target in last
    place, and second handle symlink creation failure the same way as a new item
    creation failure.

    Signed-off-by: Louis Rilling
    ---
    fs/configfs/symlink.c | 39 +++++++++++++++++++++++++++++----------
    1 files changed, 29 insertions(+), 10 deletions(-)

    diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
    index 58722a9..0c5e650 100644
    --- a/fs/configfs/symlink.c
    +++ b/fs/configfs/symlink.c
    @@ -69,6 +69,7 @@ static int create_link(struct config_item *parent_item,
    struct config_item *item,
    struct dentry *dentry)
    {
    + struct configfs_dirent *parent_sd = parent_item->ci_dentry->d_fsdata;
    struct configfs_dirent *target_sd = item->ci_dentry->d_fsdata;
    struct configfs_symlink *sl;
    int ret;
    @@ -77,24 +78,42 @@ static int create_link(struct config_item *parent_item,
    sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
    if (sl) {
    sl->sl_target = config_item_get(item);
    +
    spin_lock(&configfs_dirent_lock);
    - if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
    - spin_unlock(&configfs_dirent_lock);
    - config_item_put(item);
    - kfree(sl);
    - return -EPERM;
    - }
    - list_add(&sl->sl_list, &target_sd->s_links);
    + /*
    + * Force rmdir() of parent_item to wait until we know if we
    + * succeed.
    + */
    + parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
    spin_unlock(&configfs_dirent_lock);
    +
    ret = configfs_create_link(sl, parent_item->ci_dentry,
    dentry);
    - if (ret) {
    - spin_lock(&configfs_dirent_lock);
    - list_del_init(&sl->sl_list);
    +
    + spin_lock(&configfs_dirent_lock);
    + parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
    +
    + if (ret || target_sd->s_type & CONFIGFS_USET_DROPPING) {
    + struct configfs_dirent *sd = NULL;
    +
    + if (!ret) {
    + sd = dentry->d_fsdata;
    + list_del_init(&sd->s_sibling);
    + }
    spin_unlock(&configfs_dirent_lock);
    +
    + if (!ret) {
    + configfs_drop_dentry(sd, dentry->d_parent);
    + d_delete(dentry);
    + configfs_put(sd);
    + }
    config_item_put(item);
    kfree(sl);
    + return ret ? ret : -EPERM;
    }
    +
    + list_add(&sl->sl_list, &target_sd->s_links);
    + spin_unlock(&configfs_dirent_lock);
    }

    return ret;
    --
    1.5.5.3

    --
    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/

  5. Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

    On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:
    > For the parent's rmdir() case, we can use the same solution as with mkdir() vs
    > rmdir(). For the target's rmdir() case, we cannot, since we do not and cannot
    > lock the target's inode while in symlink(). Fortunately, once create_link()
    > terminates, no further operation can fail in symlink(). So we first reorder the
    > operations in create_link() to attach the new symlink to its target in last
    > place, and second handle symlink creation failure the same way as a new item
    > creation failure.


    Oh, no, ugh. We don't want to create vfs objects first and ask
    questions later. Otherwise we wouldn't need ATTACHING - we'd just
    create the symlink, then check dropping.
    If you have ATTACHING set, the rmdir cannot continue - you can
    check dropping at that time. That is, you keep the DROPPING check where
    it is - if it is already set, you know that rmdir() is going to complete
    successfully. You can bail before even calling configfs_create_link().
    If, however, it isn't set, your ATTACHING protects you from rmdir
    throughout.

    sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
    if (sl) {
    sl->sl_target = config_item_get(item);
    spin_lock(&configfs_dirent_lock);
    if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
    spin_unlock(&configfs_dirent_lock);
    config_item_put(item);
    kfree(sl);
    return -ENOENT;
    /*
    * Force rmdir() of parent_item to wait until we know
    * if we succeed.
    */
    parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
    }
    list_add(&sl->sl_list, &target_sd->s_links);
    spin_unlock(&configfs_dirent_lock);
    ret = configfs_create_link(sl, parent_item->ci_dentry,
    dentry);
    spin_lock(&configfs_dirent_lock);
    parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
    if (ret) {
    list_del_init(&sl->sl_list);
    spin_unlock(&configfs_dirent_lock);
    config_item_put(item);
    kfree(sl);
    } else
    spin_unlock(&configfs_dirent_lock);
    }

    return ret;

    --

    "When arrows don't penetrate, see...
    Cupid grabs the pistol."

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127
    --
    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/

  6. Re: [BUGFIX][PATCH 1/3] configfs: Fix symlink() to a removing item

    On Tue, Jun 17, 2008 at 07:37:21PM +0200, Louis Rilling wrote:
    > This patch makes configfs_symlink() fail whenever the target is being removed,
    > using the CONFIGFS_USET_DROPPING flag set by configfs_detach_prep() and
    > protected by configfs_dirent_lock.
    > diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
    > index faeb441..58722a9 100644
    > --- a/fs/configfs/symlink.c
    > +++ b/fs/configfs/symlink.c
    > @@ -78,6 +78,12 @@ static int create_link(struct config_item *parent_item,
    > if (sl) {
    > sl->sl_target = config_item_get(item);
    > spin_lock(&configfs_dirent_lock);
    > + if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
    > + spin_unlock(&configfs_dirent_lock);
    > + config_item_put(item);
    > + kfree(sl);
    > + return -EPERM;


    This needs to be -ENOENT. If rmdir set DROPPING and then
    released dirent_lock, it's going to remove the target. That's ENOENT.

    Joel

    --

    "Sometimes when reading Goethe I have the paralyzing suspicion
    that he is trying to be funny."
    - Guy Davenport

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127
    --
    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/

  7. Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

    On Tue, Jun 17, 2008 at 03:15:28PM -0700, Joel Becker wrote:
    > On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:
    > > For the parent's rmdir() case, we can use the same solution as with mkdir() vs
    > > rmdir(). For the target's rmdir() case, we cannot, since we do not and cannot
    > > lock the target's inode while in symlink(). Fortunately, once create_link()
    > > terminates, no further operation can fail in symlink(). So we first reorder the
    > > operations in create_link() to attach the new symlink to its target in last
    > > place, and second handle symlink creation failure the same way as a newitem
    > > creation failure.

    >
    > Oh, no, ugh. We don't want to create vfs objects first and ask
    > questions later. Otherwise we wouldn't need ATTACHING - we'd just
    > create the symlink, then check dropping.
    > If you have ATTACHING set, the rmdir cannot continue - you can
    > check dropping at that time. That is, you keep the DROPPING check where
    > it is - if it is already set, you know that rmdir() is going to complete
    > successfully. You can bail before even calling configfs_create_link().
    > If, however, it isn't set, your ATTACHING protects you from rmdir
    > throughout.


    The problem is rmdir() of the target item (see below). ATTACHING only protects
    us from rmdir() of the parent. This is the exact reason why I attach the link to
    the target in last place, where we know that we won't have to rollback.
    And AFAICS, creating a VFS object can not hurt as long as we hold the
    parent i_mutex, right? Otherwise there already is a problem in
    configfs_attach_item() where a failure in populate_attrs() leads to rollback the
    creation of the VFS object already created for the item.

    >
    > sl = kmalloc(sizeof(struct configfs_symlink), GFP_KERNEL);
    > if (sl) {
    > sl->sl_target = config_item_get(item);
    > spin_lock(&configfs_dirent_lock);
    > if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
    > spin_unlock(&configfs_dirent_lock);
    > config_item_put(item);
    > kfree(sl);
    > return -ENOENT;
    > /*
    > * Force rmdir() of parent_item to wait until we know
    > * if we succeed.
    > */
    > parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
    > }
    > list_add(&sl->sl_list, &target_sd->s_links);
    > spin_unlock(&configfs_dirent_lock);
    > ret = configfs_create_link(sl, parent_item->ci_dentry,
    > dentry);
    > spin_lock(&configfs_dirent_lock);
    > parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
    > if (ret) {


    Here, if detach_prep() of the target failed because of the link attached above,
    it had no means to retry. rmdir() of the target fails because of this
    temporary link, which results in a failing symlink() making rmdir() of the
    target fail.

    > list_del_init(&sl->sl_list);
    > spin_unlock(&configfs_dirent_lock);
    > config_item_put(item);
    > kfree(sl);
    > } else
    > spin_unlock(&configfs_dirent_lock);
    > }
    >
    > return ret;
    >


    --
    Dr Louis Rilling Kerlabs
    Skype: louis.rilling Batiment Germanium
    Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
    http://www.kerlabs.com/ 35700 Rennes

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.6 (GNU/Linux)

    iD8DBQFIWPQ7VKcRuvQ9Q1QRAvstAJ9b1QhYHTdZLYwD3XMVJE V2VvnHRgCghEnL
    0aZ98ZQ/tXIDob0MfuQNQyc=
    =KQ34
    -----END PGP SIGNATURE-----


  8. Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

    On Wed, Jun 18, 2008 at 01:40:43PM +0200, Louis Rilling wrote:
    > The problem is rmdir() of the target item (see below). ATTACHING only protects
    > us from rmdir() of the parent. This is the exact reason why I attach the link to
    > the target in last place, where we know that we won't have to rollback.


    Why wouldn't it protect the target, given that detach_prep()
    will be called against the target if it's being rmdir'd?

    > And AFAICS, creating a VFS object can not hurt as long as we hold the
    > parent i_mutex, right? Otherwise there already is a problem in
    > configfs_attach_item() where a failure in populate_attrs() leads to rollback the
    > creation of the VFS object already created for the item.


    We *can* do that, but we try to isolate it - hand-building VFS
    objects is complex and error prone, and I try to isolate that to
    specific cases. I'd rather avoid it when not necessary.

    > > spin_lock(&configfs_dirent_lock);
    > > parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
    > > if (ret) {

    >
    > Here, if detach_prep() of the target failed because of the link attached above,
    > it had no means to retry. rmdir() of the target fails because of this
    > temporary link, which results in a failing symlink() making rmdir() of the
    > target fail.


    How so? It sees ATTACHING, it gets -EAGAIN, it tries again,
    just like before. What's different?

    Joel

    --

    "Anything that is too stupid to be spoken is sung."
    - Voltaire

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127
    --
    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/

  9. Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

    On Wed, Jun 18, 2008 at 01:11:07PM -0700, Joel Becker wrote:
    > On Wed, Jun 18, 2008 at 01:40:43PM +0200, Louis Rilling wrote:
    > > The problem is rmdir() of the target item (see below). ATTACHING only protects
    > > us from rmdir() of the parent. This is the exact reason why I attach the link to
    > > the target in last place, where we know that we won't have to rollback.

    >
    > Why wouldn't it protect the target, given that detach_prep()
    > will be called against the target if it's being rmdir'd?


    Because
    1/ setting and clearing ATTACHING could badly interact with mkdir()/symlink()
    inside the target item (for instance clear the flag before mkdir() has finished
    attaching a new item); to avoid this we could use a different flag, but
    2/ rmdir() of the target cannot lock the inode of the new symlink's parent like
    it does for mkdir(), otherwise we would risk a deadlock with other symlink() and
    sys_rename(). This means that rmdir() should retry aggressively, in a busy
    waiting loop, or replacing mutex_lock()/mutex_unlock() with yield().

    >
    > > And AFAICS, creating a VFS object can not hurt as long as we hold the
    > > parent i_mutex, right? Otherwise there already is a problem in
    > > configfs_attach_item() where a failure in populate_attrs() leads to rollback the
    > > creation of the VFS object already created for the item.

    >
    > We *can* do that, but we try to isolate it - hand-building VFS
    > objects is complex and error prone, and I try to isolate that to
    > specific cases. I'd rather avoid it when not necessary.


    In the case of symlink(), building a new inode is what all filesystems mustdo.
    The only "bad" side-effect I can figure out of having to rollback is that the
    new entry will be visible for a short time until it is removed.

    Anyway, do you think that the "solutions" above are more acceptable?

    >
    > > > spin_lock(&configfs_dirent_lock);
    > > > parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
    > > > if (ret) {

    > >
    > > Here, if detach_prep() of the target failed because of the link attached above,
    > > it had no means to retry. rmdir() of the target fails because of this
    > > temporary link, which results in a failing symlink() making rmdir() of the
    > > target fail.

    >
    > How so? It sees ATTACHING, it gets -EAGAIN, it tries again,
    > just like before. What's different?


    See above the reasons for not using ATTACHING on the target.

    Louis

    --
    Dr Louis Rilling Kerlabs
    Skype: louis.rilling Batiment Germanium
    Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
    http://www.kerlabs.com/ 35700 Rennes

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.6 (GNU/Linux)

    iD8DBQFIWibKVKcRuvQ9Q1QRAvYKAJ0Td13sLHxqTC15Z053MU dj/dO4PwCcCOrx
    Xctu7LQhb2IkwUOiKqyQjfY=
    =0x6/
    -----END PGP SIGNATURE-----


  10. Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

    On Thu, Jun 19, 2008 at 11:28:42AM +0200, Louis Rilling wrote:
    > On Wed, Jun 18, 2008 at 01:11:07PM -0700, Joel Becker wrote:
    > > On Wed, Jun 18, 2008 at 01:40:43PM +0200, Louis Rilling wrote:
    > > > The problem is rmdir() of the target item (see below). ATTACHING only protects
    > > > us from rmdir() of the parent. This is the exact reason why I attach the link to
    > > > the target in last place, where we know that we won't have to rollback.

    > >
    > > Why wouldn't it protect the target, given that detach_prep()
    > > will be called against the target if it's being rmdir'd?

    >
    > Because
    > 1/ setting and clearing ATTACHING could badly interact with mkdir()/symlink()
    > inside the target item (for instance clear the flag before mkdir() has finished
    > attaching a new item); to avoid this we could use a different flag, but


    Ok, true, we don't have a lock to protect mkdir.

    > 2/ rmdir() of the target cannot lock the inode of the new symlink's parent like
    > it does for mkdir(), otherwise we would risk a deadlock with other symlink() and
    > sys_rename(). This means that rmdir() should retry aggressively, in a busy
    > waiting loop, or replacing mutex_lock()/mutex_unlock() with yield().


    Yup, we'd have to have some other form of retry - note that this
    is all spinlock territory. Thus, it should be fast. By the time
    rmdir() gets back out to the toplevel, symlink/mkdir should be done
    creating whatever they needed and waiting on the dirnet_lock. Then
    rmdir waits again on the lock. It "should" be bang-bang.
    Yes, I know, assumptions all around.

    > > We *can* do that, but we try to isolate it - hand-building VFS
    > > objects is complex and error prone, and I try to isolate that to
    > > specific cases. I'd rather avoid it when not necessary.

    >
    > In the case of symlink(), building a new inode is what all filesystems must do.
    > The only "bad" side-effect I can figure out of having to rollback is that the
    > new entry will be visible for a short time until it is removed.


    It won't be visible, because we hold i_mutex until we're done.

    > Anyway, do you think that the "solutions" above are more acceptable?


    The code for create then destroy was quite ugly. Maybe it
    struck me because of that.

    Joel

    --

    print STDOUT q
    Just another Perl hacker,
    unless $spring
    - Larry Wall

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127
    --
    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/

  11. Re: [BUGFIX][PATCH 3/3] configfs: Fix failing symlink() making rmdir() fail

    On Tue, Jun 17, 2008 at 07:37:23PM +0200, Louis Rilling wrote:

    Ok, I can see some of why I hated this.

    > +
    > spin_lock(&configfs_dirent_lock);
    > - if (target_sd->s_type & CONFIGFS_USET_DROPPING) {
    > - spin_unlock(&configfs_dirent_lock);
    > - config_item_put(item);
    > - kfree(sl);
    > - return -EPERM;
    > - }
    > - list_add(&sl->sl_list, &target_sd->s_links);
    > + /*
    > + * Force rmdir() of parent_item to wait until we know if we
    > + * succeed.
    > + */
    > + parent_sd->s_type |= CONFIGFS_USET_ATTACHING;
    > spin_unlock(&configfs_dirent_lock);
    > +
    > ret = configfs_create_link(sl, parent_item->ci_dentry,
    > dentry);
    > - if (ret) {
    > - spin_lock(&configfs_dirent_lock);
    > - list_del_init(&sl->sl_list);
    > +
    > + spin_lock(&configfs_dirent_lock);
    > + parent_sd->s_type &= ~CONFIGFS_USET_ATTACHING;
    > +
    > + if (ret || target_sd->s_type & CONFIGFS_USET_DROPPING) {


    Use parenthesis. Actually, separate out the error cases.

    > + struct configfs_dirent *sd = NULL;
    > +
    > + if (!ret) {
    > + sd = dentry->d_fsdata;
    > + list_del_init(&sd->s_sibling);
    > + }
    > spin_unlock(&configfs_dirent_lock);
    > +
    > + if (!ret) {
    > + configfs_drop_dentry(sd, dentry->d_parent);
    > + d_delete(dentry);
    > + configfs_put(sd);
    > + }


    This open-code of the VFS munging is ripe to break when the VFS
    changes or other configfs changes happen. The real issue is that you
    are reimplementing the core of configfs_unlink(). Note how the core VFS
    munging of configfs_rmdir() is separated out so that configfs_mkdir()
    can also use it in the failure case? Do the same with unlink() and it
    will read much better ("if (DROPPING) configfs_delete_link()"). Call it
    configfs_remove_link() or configfs_delete_link().

    Joel

    --

    Life's Little Instruction Book #337

    "Reread your favorite book."

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127
    --
    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/

  12. [PATCH] configfs: Fix failing symlink() making rmdir() fail

    Joel,

    What about this other solution? By the way, it does not even need to
    rename CONFIGFS_USET_IN_MKDIR.

    Louis

    --
    Dr Louis Rilling Kerlabs
    Skype: louis.rilling Batiment Germanium
    Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
    http://www.kerlabs.com/ 35700 Rennes


  13. Re: [PATCH] configfs: Fix failing symlink() making rmdir() fail

    On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
    > What about this other solution? By the way, it does not even need to
    > rename CONFIGFS_USET_IN_MKDIR.


    We don't need the check for USET_DROPPING either. I like a lot!

    Joel

    --

    "I don't want to achieve immortality through my work; I want to
    achieve immortality through not dying."
    - Woody Allen

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127
    --
    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/

  14. Re: [PATCH] configfs: Fix failing symlink() making rmdir() fail

    On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
    > What about this other solution? By the way, it does not even need to
    > rename CONFIGFS_USET_IN_MKDIR.


    I like it. My first thought was "why doesn't he take the mutex
    during configfs_unlink()?", but the unlink is protected enough by
    dirent_lock.

    Joel

    --

    "Reality is merely an illusion, albeit a very persistent one."
    - Albert Einstien

    Joel Becker
    Principal Software Developer
    Oracle
    E-mail: joel.becker@oracle.com
    Phone: (650) 506-8127
    --
    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/

  15. Re: [PATCH] configfs: Fix failing symlink() making rmdir() fail

    On Fri, Jun 20, 2008 at 03:44:20PM -0700, Joel Becker wrote:
    > On Fri, Jun 20, 2008 at 02:09:22PM +0200, Louis Rilling wrote:
    > > What about this other solution? By the way, it does not even need to
    > > rename CONFIGFS_USET_IN_MKDIR.

    >
    > We don't need the check for USET_DROPPING either. I like a lot!


    I hope that you wasn't speaking about checking USET_DROPPING on the target.We
    still need to check it, of course. I'm resending the updated, hopefully final,
    patchset.

    Louis

    --
    Dr Louis Rilling Kerlabs
    Skype: louis.rilling Batiment Germanium
    Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
    http://www.kerlabs.com/ 35700 Rennes

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.6 (GNU/Linux)

    iD8DBQFIX5GcVKcRuvQ9Q1QRApfwAKCOXgWUX+MXZBHGuwgzZl naFXj10QCgiG3O
    oV5X0Tw4s4nGny0IWbADMrU=
    =4DoZ
    -----END PGP SIGNATURE-----


+ Reply to Thread