Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210709071100.4057639-1-yangyingliang@huawei.com> <YOx392cwdEHMMnD0@slm.duckdns.org>
In-Reply-To: <YOx392cwdEHMMnD0@slm.duckdns.org>
From:   Lai Jiangshan <jiangshanlai@gmail.com>
Date:   Tue, 13 Jul 2021 13:56:12 +0800
Message-ID: <CAJhGHyC+oyX6fqnR1-JPnNgPuDpikU6KYr1iXrj7BDZHnjPGTA@mail.gmail.com>
Subject: Re: [PATCH v2] workqueue: fix UAF in pwq_unbound_release_workfn()
To:     Tejun Heo <tj@kernel.org>
Cc:     Yang Yingliang <yangyingliang@huawei.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Xu Qiang <xuqiang36@huawei.com>,
        Pavel Skripkin <paskripkin@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 13, 2021 at 1:12 AM Tejun Heo <tj@kernel.org> wrote:
>
> Hello, Yang.
>
> > +static void free_pwq(struct pool_workqueue *pwq)
> > +{
> > +     if (!pwq || --pwq->refcnt)
> > +             return;
> > +
> > +     put_unbound_pool(pwq->pool);
> > +     kmem_cache_free(pwq_cache, pwq);
> > +}
> > +
> > +static void free_wqattrs_ctx(struct apply_wqattrs_ctx *ctx)
> > +{
> > +     int node;
> > +
> > +     if (!ctx)
> > +             return;
> > +
> > +     for_each_node(node)
> > +             free_pwq(ctx->pwq_tbl[node]);
> > +     free_pwq(ctx->dfl_pwq);
> > +
> > +     free_workqueue_attrs(ctx->attrs);
> > +
> > +     kfree(ctx);
> > +}
>
> It bothers me that we're partially replicating the free path including pwq
> refcnting.

The replicating code can be reduced by merging
apply_wqattrs_cleanup() into apply_wqattrs_commit().

> Does something like the following work?

It works since it has a flush_scheduled_work() in
alloc_and_link_pwqs(). But I don't think it works for
workqueue_apply_unbound_cpumask() when apply_wqattrs_commit()
is not called.

If we want to reuse the current apply_wqattrs_cleanup(), I would prefer
something like this: (untested)

@@ -3680,15 +3676,21 @@ static void pwq_unbound_release_workfn(struct
work_struct *work)
                                                  unbound_release_work);
        struct workqueue_struct *wq = pwq->wq;
        struct worker_pool *pool = pwq->pool;
-       bool is_last;
+       bool is_last = false;

-       if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
-               return;
+       /*
+        * when @pwq is not linked, it doesn't hold any reference to the
+        * @wq, and @wq is invalid to access.
+        */
+       if (!list_empty(&pwq->pwqs_node)) {
+               if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
+                       return;

-       mutex_lock(&wq->mutex);
-       list_del_rcu(&pwq->pwqs_node);
-       is_last = list_empty(&wq->pwqs);
-       mutex_unlock(&wq->mutex);
+               mutex_lock(&wq->mutex);
+               list_del_rcu(&pwq->pwqs_node);
+               is_last = list_empty(&wq->pwqs);
+               mutex_unlock(&wq->mutex);
+       }

        mutex_lock(&wq_pool_mutex);
        put_unbound_pool(pool);

>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 104e3ef04e33..0c0ab363edeb 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3693,7 +3693,7 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
>          * If we're the last pwq going away, @wq is already dead and no one
>          * is gonna access it anymore.  Schedule RCU free.
>          */
> -       if (is_last) {
> +       if (is_last && !list_empty(&wq->list)) {
>                 wq_unregister_lockdep(wq);
>                 call_rcu(&wq->rcu, rcu_free_wq);
>         }
> @@ -4199,6 +4199,10 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
>         }
>         put_online_cpus();
>
> +       if (ret) {
> +               flush_scheduled_work();
> +       }
> +
>         return ret;
>  }
>
> --
> tejun