| Message ID | 20211013093016.3869299-1-wanghai38@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | IB/cm: Fix possible use-after-free in ib_cm_cleanup() | expand |
On Wed, Oct 13, 2021 at 05:30:16PM +0800, Wang Hai wrote: > This module's remove path calls cancel_delayed_work(). However, that > function does not wait until the work function finishes. This means > that the callback function may still be running after the driver's > remove function has finished, which would result in a use-after-free. > > Fix by calling cancel_delayed_work_sync(), which ensures that > the work is properly cancelled, no longer running, and unable > to re-schedule itself. > > Fixes: 8575329d4f85 ("IB/cm: Fix timewait crash after module unload") > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Wang Hai <wanghai38@huawei.com> > drivers/infiniband/core/cm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index c903b74f46a4..ae0af63f3271 100644 > +++ b/drivers/infiniband/core/cm.c > @@ -4508,7 +4508,7 @@ static void __exit ib_cm_cleanup(void) > > spin_lock_irq(&cm.lock); > list_for_each_entry(timewait_info, &cm.timewait_list, list) > - cancel_delayed_work(&timewait_info->work.work); > + cancel_delayed_work_sync(&timewait_info->work.work); > spin_unlock_irq(&cm.lock); No, this will deadlock: static int cm_timewait_handler(struct cm_work *work) { struct cm_timewait_info *timewait_info; struct cm_id_private *cm_id_priv; timewait_info = container_of(work, struct cm_timewait_info, work); spin_lock_irq(&cm.lock); ^^^^^^^^^^^^ Holds the same lock What is your bug? The destroy_wq() a few lines below will flush out all the work so it is already not possible that work can still exist after the driver's remove function has finished. Jason
在 2021/10/14 2:24, Jason Gunthorpe 写道: > On Wed, Oct 13, 2021 at 05:30:16PM +0800, Wang Hai wrote: >> This module's remove path calls cancel_delayed_work(). However, that >> function does not wait until the work function finishes. This means >> that the callback function may still be running after the driver's >> remove function has finished, which would result in a use-after-free. >> >> Fix by calling cancel_delayed_work_sync(), which ensures that >> the work is properly cancelled, no longer running, and unable >> to re-schedule itself. >> >> Fixes: 8575329d4f85 ("IB/cm: Fix timewait crash after module unload") >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Wang Hai <wanghai38@huawei.com> >> drivers/infiniband/core/cm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c >> index c903b74f46a4..ae0af63f3271 100644 >> +++ b/drivers/infiniband/core/cm.c >> @@ -4508,7 +4508,7 @@ static void __exit ib_cm_cleanup(void) >> >> spin_lock_irq(&cm.lock); >> list_for_each_entry(timewait_info, &cm.timewait_list, list) >> - cancel_delayed_work(&timewait_info->work.work); >> + cancel_delayed_work_sync(&timewait_info->work.work); >> spin_unlock_irq(&cm.lock); > No, this will deadlock: > > static int cm_timewait_handler(struct cm_work *work) > { > struct cm_timewait_info *timewait_info; > struct cm_id_private *cm_id_priv; > > timewait_info = container_of(work, struct cm_timewait_info, work); > spin_lock_irq(&cm.lock); > ^^^^^^^^^^^^ > > Holds the same lock > > What is your bug? The destroy_wq() a few lines below will flush out > all the work so it is already not possible that work can still exist > after the driver's remove function has finished. > > Jason > . Sorry, this is a wrong bugfix, thank you for pointing it out. I was studying the code here and thought there might be a null pointer reference problem. You are right, I didn't take into account destroy_workqueue(). There are no bugs here. sorry for making this problematic patch. Please ignore this patch.
diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index c903b74f46a4..ae0af63f3271 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -4508,7 +4508,7 @@ static void __exit ib_cm_cleanup(void) spin_lock_irq(&cm.lock); list_for_each_entry(timewait_info, &cm.timewait_list, list) - cancel_delayed_work(&timewait_info->work.work); + cancel_delayed_work_sync(&timewait_info->work.work); spin_unlock_irq(&cm.lock); ib_unregister_client(&cm_client);
This module's remove path calls cancel_delayed_work(). However, that function does not wait until the work function finishes. This means that the callback function may still be running after the driver's remove function has finished, which would result in a use-after-free. Fix by calling cancel_delayed_work_sync(), which ensures that the work is properly cancelled, no longer running, and unable to re-schedule itself. Fixes: 8575329d4f85 ("IB/cm: Fix timewait crash after module unload") Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Wang Hai <wanghai38@huawei.com> --- drivers/infiniband/core/cm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)