lkml.org 
[lkml]   [2022]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.4 383/411] kernfs: Separate kernfs_pr_cont_buf and rename_lock.
    Date
    From: Hao Luo <haoluo@google.com>

    [ Upstream commit 1a702dc88e150487c9c173a249b3d236498b9183 ]

    Previously the protection of kernfs_pr_cont_buf was piggy backed by
    rename_lock, which means that pr_cont() needs to be protected under
    rename_lock. This can cause potential circular lock dependencies.

    If there is an OOM, we have the following call hierarchy:

    -> cpuset_print_current_mems_allowed()
    -> pr_cont_cgroup_name()
    -> pr_cont_kernfs_name()

    pr_cont_kernfs_name() will grab rename_lock and call printk. So we have
    the following lock dependencies:

    kernfs_rename_lock -> console_sem

    Sometimes, printk does a wakeup before releasing console_sem, which has
    the dependence chain:

    console_sem -> p->pi_lock -> rq->lock

    Now, imagine one wants to read cgroup_name under rq->lock, for example,
    printing cgroup_name in a tracepoint in the scheduler code. They will
    be holding rq->lock and take rename_lock:

    rq->lock -> kernfs_rename_lock

    Now they will deadlock.

    A prevention to this circular lock dependency is to separate the
    protection of pr_cont_buf from rename_lock. In principle, rename_lock
    is to protect the integrity of cgroup name when copying to buf. Once
    pr_cont_buf has got its content, rename_lock can be dropped. So it's
    safe to drop rename_lock after kernfs_name_locked (and
    kernfs_path_from_node_locked) and rely on a dedicated pr_cont_lock
    to protect pr_cont_buf.

    Acked-by: Tejun Heo <tj@kernel.org>
    Signed-off-by: Hao Luo <haoluo@google.com>
    Link: https://lore.kernel.org/r/20220516190951.3144144-1-haoluo@google.com
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    fs/kernfs/dir.c | 31 +++++++++++++++++++------------
    1 file changed, 19 insertions(+), 12 deletions(-)

    diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
    index 7d4af6cea2a6..99ee657596b5 100644
    --- a/fs/kernfs/dir.c
    +++ b/fs/kernfs/dir.c
    @@ -19,7 +19,15 @@

    DEFINE_MUTEX(kernfs_mutex);
    static DEFINE_SPINLOCK(kernfs_rename_lock); /* kn->parent and ->name */
    -static char kernfs_pr_cont_buf[PATH_MAX]; /* protected by rename_lock */
    +/*
    + * Don't use rename_lock to piggy back on pr_cont_buf. We don't want to
    + * call pr_cont() while holding rename_lock. Because sometimes pr_cont()
    + * will perform wakeups when releasing console_sem. Holding rename_lock
    + * will introduce deadlock if the scheduler reads the kernfs_name in the
    + * wakeup path.
    + */
    +static DEFINE_SPINLOCK(kernfs_pr_cont_lock);
    +static char kernfs_pr_cont_buf[PATH_MAX]; /* protected by pr_cont_lock */
    static DEFINE_SPINLOCK(kernfs_idr_lock); /* root->ino_idr */

    #define rb_to_kn(X) rb_entry((X), struct kernfs_node, rb)
    @@ -230,12 +238,12 @@ void pr_cont_kernfs_name(struct kernfs_node *kn)
    {
    unsigned long flags;

    - spin_lock_irqsave(&kernfs_rename_lock, flags);
    + spin_lock_irqsave(&kernfs_pr_cont_lock, flags);

    - kernfs_name_locked(kn, kernfs_pr_cont_buf, sizeof(kernfs_pr_cont_buf));
    + kernfs_name(kn, kernfs_pr_cont_buf, sizeof(kernfs_pr_cont_buf));
    pr_cont("%s", kernfs_pr_cont_buf);

    - spin_unlock_irqrestore(&kernfs_rename_lock, flags);
    + spin_unlock_irqrestore(&kernfs_pr_cont_lock, flags);
    }

    /**
    @@ -249,10 +257,10 @@ void pr_cont_kernfs_path(struct kernfs_node *kn)
    unsigned long flags;
    int sz;

    - spin_lock_irqsave(&kernfs_rename_lock, flags);
    + spin_lock_irqsave(&kernfs_pr_cont_lock, flags);

    - sz = kernfs_path_from_node_locked(kn, NULL, kernfs_pr_cont_buf,
    - sizeof(kernfs_pr_cont_buf));
    + sz = kernfs_path_from_node(kn, NULL, kernfs_pr_cont_buf,
    + sizeof(kernfs_pr_cont_buf));
    if (sz < 0) {
    pr_cont("(error)");
    goto out;
    @@ -266,7 +274,7 @@ void pr_cont_kernfs_path(struct kernfs_node *kn)
    pr_cont("%s", kernfs_pr_cont_buf);

    out:
    - spin_unlock_irqrestore(&kernfs_rename_lock, flags);
    + spin_unlock_irqrestore(&kernfs_pr_cont_lock, flags);
    }

    /**
    @@ -870,13 +878,12 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,

    lockdep_assert_held(&kernfs_mutex);

    - /* grab kernfs_rename_lock to piggy back on kernfs_pr_cont_buf */
    - spin_lock_irq(&kernfs_rename_lock);
    + spin_lock_irq(&kernfs_pr_cont_lock);

    len = strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));

    if (len >= sizeof(kernfs_pr_cont_buf)) {
    - spin_unlock_irq(&kernfs_rename_lock);
    + spin_unlock_irq(&kernfs_pr_cont_lock);
    return NULL;
    }

    @@ -888,7 +895,7 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,
    parent = kernfs_find_ns(parent, name, ns);
    }

    - spin_unlock_irq(&kernfs_rename_lock);
    + spin_unlock_irq(&kernfs_pr_cont_lock);

    return parent;
    }
    --
    2.35.1


    \
     
     \ /
      Last update: 2022-06-13 13:49    [W:4.140 / U:0.532 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site