Re: [PATCH] xen/scsiback: unbind the IRQ before draining in-flight requests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] xen/scsiback: premature IRQ unbind in scsiback_disconnect()
  races with responses
--

commit 3923ae443e53c8e678c6827dafcd0fcc2ee96487
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

xen/scsiback: unbind the IRQ before draining in-flight requests

This commit moves unbind_from_irqhandler() and clearing info->irq ahead of
waiting for nr_unreplied_reqs to reach zero in scsiback_disconnect() so the
threaded IRQ handler cannot admit new requests after the drain finishes.

> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index c7036e0e41bda..647a3c7e2a2b3 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -603,11 +603,13 @@ static int scsiback_gnttab_data_map(struct vscsiif_request *ring_req,
>  
>  static void scsiback_disconnect(struct vscsibk_info *info)
>  {
> +	/* Stop admitting requests before draining the in-flight ones. */
> +	unbind_from_irqhandler(info->irq, info);
> +	info->irq = 0;
> +
>  	wait_event(info->waiting_to_free,
>  		atomic_read(&info->nr_unreplied_reqs) == 0);
>  
> -	unbind_from_irqhandler(info->irq, info);
> -	info->irq = 0;
>  	xenbus_unmap_ring_vfree(info->dev, info->ring.sring);
>  }

[Severity: High]
Does unbinding and zeroing info->irq before wait_event() introduce a
regression where scsiback_send_response() races with delayed_free_irq()
or notifies IRQ 0 while in-flight requests are still draining?

In-flight commands complete on target_completion workqueues via
scsiback_cmd_done(), which calls scsiback_do_resp_with_sense() ->
scsiback_send_response() before scsiback_put(info) decrements
info->nr_unreplied_reqs.  In scsiback_send_response(), info->irq is read
outside info->ring_lock and passed to notify_remote_via_irq() in
preemptible context without RCU protection:

CPU0                              CPU1 (kworker)
scsiback_disconnect()             scsiback_cmd_done()
                                    ...
                                    scsiback_send_response()
                                      irq = info->irq;
  unbind_from_irqhandler(...)
    ...
    xen_free_irq(irq)
      queue_rcu_work(..., &rwork)
  info->irq = 0;
  wait_event(..., nr_unreplied_reqs == 0)
  [RCU grace period elapses]
  delayed_free_irq()
    set_info_for_irq(irq, NULL)
    kfree(irq_info)
    xen_irq_free_desc(irq)
                                      notify_remote_via_irq(irq)
                                        evtchn_from_irq(irq)
                                          irq_info->evtchn <-- use-after-free
                                    scsiback_put(info)

Once info->irq = 0 becomes visible during wait_event(), every completing
request calls notify_remote_via_irq(0).  IRQ 0 is a valid Xen IRQ number:
xen_allocate_irq_dynamic() allocates descriptors via
irq_alloc_desc_from(0, -1), so on platforms where nr_legacy_irqs() is 0
(such as ARM64 or x86 Xen PV domains with null_legacy_pic, where
VIRQ_TIMER binds to IRQ 0 at boot), info_for_irq(0) is non-NULL with a
valid event channel and notify_remote_via_irq(0) sends spurious
notifications to that channel.

Unbinding before wait_event() also closes the event channel via
xen_evtchn_close() before in-flight responses are pushed to the ring,
dropping completion notifications during the drain.  Calling
disable_irq(info->irq) before wait_event() and keeping
unbind_from_irqhandler(info->irq, info) after wait_event() would stop
scsiback_irq_fn() synchronously while keeping info->irq valid until all
in-flight requests have completed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109122654.30859-1-yhlee@isslab.korea.ac.kr?part=1