Re: [Devel] [PATCH hci-8.1 v4 09/11] dm-ploop: fix use-after-free on the last inflight_md decrement

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dm-ploop: ploop_destroy() self-deadlocks using wait_event_lock_irq() on wq.lock
--

commit e7aad5678d9b3ec2b2234a7e4beccd4508f457af
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: fix use-after-free on the last inflight_md decrement

This patch moves the inflight_md decrement and wakeup to the end of the
piwb completion path, so that it is the last access to the ploop. The
decrement and wakeup are done under inflight_md_wq.lock, the waiters check
the counter under the same lock, and ploop_destroy() now waits for
inflight_md to reach zero before freeing anything.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-ploop-target.c b/drivers/md/dm-ploop-target.c
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c

[ ... ]

> @@ -192,6 +192,13 @@ static void ploop_destroy(struct ploop *ploop)
>  		if (ploop->kt_allocator)
>  			kthread_stop(ploop->kt_allocator->task);
>  
> +		/* BAT writes may still be completing from aio context */
> +		spin_lock_irq(&ploop->inflight_md_wq.lock);
> +		wait_event_lock_irq(ploop->inflight_md_wq,
> +				    !atomic_read(&ploop->inflight_md),
> +				    ploop->inflight_md_wq.lock);
> +		spin_unlock_irq(&ploop->inflight_md_wq.lock);
> +

[Severity: High]
Can this deadlock on the waitqueue lock when inflight_md is non-zero?

ploop_destroy() takes inflight_md_wq.lock and then passes that same lock
as the lock argument to wait_event_lock_irq().  If the condition is
already true, the macro exits through its fast path and nothing goes
wrong.  If inflight_md is still non-zero, the call chain is:

ploop_destroy()
  spin_lock_irq(&ploop->inflight_md_wq.lock)
  wait_event_lock_irq()
    __wait_event_lock_irq()
      ___wait_event()
        prepare_to_wait_event()
          spin_lock_irqsave(&wq_head->lock, flags)

prepare_to_wait_event() acquires wq_head->lock, which is the lock the
caller already holds.  The spinlock is not recursive, so the task would
spin on itself with interrupts disabled.  The spin_unlock_irq() inside the
macro, before schedule(), is never reached.

This is the case the patch is meant to handle, namely BAT writes still
completing from aio context when ploop_destroy() runs.

wait_event_lock_irq() looks only usable when the lock argument is a
different lock from the waitqueue's own lock.  ploop_suspend_submitting_pios()
uses wait_event_interruptible_locked_irq() for this situation.

Would it work to drop the outer spin_lock_irq()/spin_unlock_irq() and use a
plain wait_event() here, since the waker already takes the waitqueue lock
in wake_up_locked()?  Alternatively, a *_locked_irq style wait could be
used, as in ploop_suspend_submitting_pios().

>  		WARN_ON(!llist_empty(&ploop->pios[PLOOP_LIST_PREPARE]));
>  		WARN_ON(!llist_empty(&ploop->llresubmit_pios));
>  		WARN_ON(!llist_empty(&ploop->enospc_pios));

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261012132646.41494-1-alexander.ivanov@virtuozzo.com?part=9

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel