On Mon, Dec 12, 2022 at 02:18:36PM +0800, Richard Clark wrote:
> Currently the __WQ_DRAINING is used to prevent a new work item from queueing
> to a draining workqueue, but this flag will be cleared before the end of a
> RCU grace period. Because the workqueue instance is actually freed after
> the RCU grace period, this fact results in an opening window in which a new
> work item can be queued into a destorying workqueue and be scheduled
> consequently, for instance, the below code snippet demos this accident:
I mean, this is just use-after-free. The same scenario can happen with
non-RCU frees or if there happens to be an RCU grace period inbetween. I'm
not sure what's being protected here.
Thanks.
--
tejun
On Mon, Dec 12, 2022 at 2:23 PM Tejun Heo <[email protected]> wrote:
>
> On Mon, Dec 12, 2022 at 02:18:36PM +0800, Richard Clark wrote:
> > Currently the __WQ_DRAINING is used to prevent a new work item from queueing
> > to a draining workqueue, but this flag will be cleared before the end of a
> > RCU grace period. Because the workqueue instance is actually freed after
> > the RCU grace period, this fact results in an opening window in which a new
> > work item can be queued into a destorying workqueue and be scheduled
> > consequently, for instance, the below code snippet demos this accident:
>
> I mean, this is just use-after-free. The same scenario can happen with
> non-RCU frees or if there happens to be an RCU grace period inbetween. I'm
> not sure what's being protected here.
I think it is a kind of debugging facility with no overhead in the
fast path.
It is indeed the caller's responsibility not to do use-after-free.
For non-RCU free, the freed workqueue's state can be arbitrary soon and
the caller might get a complaint. And if there are some kinds of debugging
facilities for freed memory, the system can notice the problem earlier.
But now is RCU free for the workqueue, and the workqueue has nothing
different between before and after destroy_workqueue() unless the
grace period ends and the memory-allocation subsystem takes charge of
the memory.
Thanks
Lai
On Mon, Dec 12, 2022 at 2:48 PM Lai Jiangshan <[email protected]> wrote:
>
> On Mon, Dec 12, 2022 at 2:23 PM Tejun Heo <[email protected]> wrote:
> >
> > On Mon, Dec 12, 2022 at 02:18:36PM +0800, Richard Clark wrote:
> > > Currently the __WQ_DRAINING is used to prevent a new work item from queueing
> > > to a draining workqueue, but this flag will be cleared before the end of a
> > > RCU grace period. Because the workqueue instance is actually freed after
> > > the RCU grace period, this fact results in an opening window in which a new
> > > work item can be queued into a destorying workqueue and be scheduled
> > > consequently, for instance, the below code snippet demos this accident:
> >
> > I mean, this is just use-after-free. The same scenario can happen with
> > non-RCU frees or if there happens to be an RCU grace period inbetween. I'm
> > not sure what's being protected here.
>
> I think it is a kind of debugging facility with no overhead in the
> fast path.
Agree...
>
> It is indeed the caller's responsibility not to do use-after-free.
>
> For non-RCU free, the freed workqueue's state can be arbitrary soon and
> the caller might get a complaint. And if there are some kinds of debugging
> facilities for freed memory, the system can notice the problem earlier.
This case will trigger a noticeable kernel BUG
>
> But now is RCU free for the workqueue, and the workqueue has nothing
> different between before and after destroy_workqueue() unless the
> grace period ends and the memory-allocation subsystem takes charge of
> the memory.
>
destroy_workqueue(wq0);
schedule_timeout_interruptible(msecs_to_jiffies(1000));
queue_work_on(1, wq0, &w0);
Sleep 1s to guarantee the RCU grace period completes, then the same
result with non-RCU free
Thanks
>
>
> Thanks
> Lai
On Mon, Dec 12, 2022 at 2:23 PM Tejun Heo <[email protected]> wrote:
>
> On Mon, Dec 12, 2022 at 02:18:36PM +0800, Richard Clark wrote:
> > Currently the __WQ_DRAINING is used to prevent a new work item from queueing
> > to a draining workqueue, but this flag will be cleared before the end of a
> > RCU grace period. Because the workqueue instance is actually freed after
> > the RCU grace period, this fact results in an opening window in which a new
> > work item can be queued into a destorying workqueue and be scheduled
> > consequently, for instance, the below code snippet demos this accident:
>
> I mean, this is just use-after-free. The same scenario can happen with
IMO, it's not exactly the use-after-free since no free action before
the end of RCU grace period, if it really is then the code will
trigger a kernel BUG:
BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
...
Which can be easily observed for both non-RCU frees and RCU frees.
Thanks
> non-RCU frees or if there happens to be an RCU grace period inbetween. I'm
> not sure what's being protected here.
>
> Thanks.
>
> --
> tejun
On Mon, Dec 12, 2022 at 02:48:25PM +0800, Lai Jiangshan wrote:
> On Mon, Dec 12, 2022 at 2:23 PM Tejun Heo <[email protected]> wrote:
> >
> > On Mon, Dec 12, 2022 at 02:18:36PM +0800, Richard Clark wrote:
> > > Currently the __WQ_DRAINING is used to prevent a new work item from queueing
> > > to a draining workqueue, but this flag will be cleared before the end of a
> > > RCU grace period. Because the workqueue instance is actually freed after
> > > the RCU grace period, this fact results in an opening window in which a new
> > > work item can be queued into a destorying workqueue and be scheduled
> > > consequently, for instance, the below code snippet demos this accident:
> >
> > I mean, this is just use-after-free. The same scenario can happen with
> > non-RCU frees or if there happens to be an RCU grace period inbetween. I'm
> > not sure what's being protected here.
>
> I think it is a kind of debugging facility with no overhead in the
> fast path.
>
> It is indeed the caller's responsibility not to do use-after-free.
>
> For non-RCU free, the freed workqueue's state can be arbitrary soon and
> the caller might get a complaint. And if there are some kinds of debugging
> facilities for freed memory, the system can notice the problem earlier.
>
> But now is RCU free for the workqueue, and the workqueue has nothing
> different between before and after destroy_workqueue() unless the
> grace period ends and the memory-allocation subsystem takes charge of
> the memory.
idk, maybe? It seems kinda out of scope. Richard, can you update the patch
description and comment so that they clearly state that this is a debug aid
to help spotting user errors?
Thanks.
--
tejun
On Tue, Dec 13, 2022 at 6:27 AM Tejun Heo <[email protected]> wrote:
>
> On Mon, Dec 12, 2022 at 02:48:25PM +0800, Lai Jiangshan wrote:
> > On Mon, Dec 12, 2022 at 2:23 PM Tejun Heo <[email protected]> wrote:
> > >
> > > On Mon, Dec 12, 2022 at 02:18:36PM +0800, Richard Clark wrote:
> > > > Currently the __WQ_DRAINING is used to prevent a new work item from queueing
> > > > to a draining workqueue, but this flag will be cleared before the end of a
> > > > RCU grace period. Because the workqueue instance is actually freed after
> > > > the RCU grace period, this fact results in an opening window in which a new
> > > > work item can be queued into a destorying workqueue and be scheduled
> > > > consequently, for instance, the below code snippet demos this accident:
> > >
> > > I mean, this is just use-after-free. The same scenario can happen with
> > > non-RCU frees or if there happens to be an RCU grace period inbetween. I'm
> > > not sure what's being protected here.
> >
> > I think it is a kind of debugging facility with no overhead in the
> > fast path.
> >
> > It is indeed the caller's responsibility not to do use-after-free.
> >
> > For non-RCU free, the freed workqueue's state can be arbitrary soon and
> > the caller might get a complaint. And if there are some kinds of debugging
> > facilities for freed memory, the system can notice the problem earlier.
> >
> > But now is RCU free for the workqueue, and the workqueue has nothing
> > different between before and after destroy_workqueue() unless the
> > grace period ends and the memory-allocation subsystem takes charge of
> > the memory.
>
> idk, maybe? It seems kinda out of scope. Richard, can you update the patch
> description and comment so that they clearly state that this is a debug aid
> to help spotting user errors?
Sure, will update soon
Thanks
>
> Thanks.
>
> --
> tejun