Messages in this thread Patch in this message | | | Date | Mon, 12 Jul 2021 07:12:23 -1000 | From | Tejun Heo <> | Subject | Re: [PATCH v2] workqueue: fix UAF in pwq_unbound_release_workfn() |
| |
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. Does something like the following work?
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
| |