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;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Tue, 13 Jul 2021 06:18:13 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Lai Jiangshan <jiangshanlai@gmail.com>
Cc:     Yang Yingliang <yangyingliang@huawei.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Xu Qiang <xuqiang36@huawei.com>,
        Pavel Skripkin <paskripkin@gmail.com>
Subject: Re: [PATCH v2] workqueue: fix UAF in pwq_unbound_release_workfn()
Message-ID: <YO28xd4mlxgN6xY8@slm.duckdns.org>
References: <20210709071100.4057639-1-yangyingliang@huawei.com>
 <YOx392cwdEHMMnD0@slm.duckdns.org>
 <CAJhGHyC+oyX6fqnR1-JPnNgPuDpikU6KYr1iXrj7BDZHnjPGTA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJhGHyC+oyX6fqnR1-JPnNgPuDpikU6KYr1iXrj7BDZHnjPGTA@mail.gmail.com>
Precedence: bulk

Hello, Lai.

On Tue, Jul 13, 2021 at 01:56:12PM +0800, Lai Jiangshan wrote:
> > 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.

Yeah, but in that path, wq is fully initialized and will always have
existing pwqs, so the wq free path shouldn't get activated. During wq
allocation, the problem is that we're installing the first set of pwqs, so
if they fail, the workqueue doesn't have any pwqs and thus triggers
self-destruction.

> 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);

But, oh yeah, this is way better.

Thanks.

-- 
tejun