Re: [PATCH RESEND] scsi: ppa: Fix use-after-free caused by unfinished delayed work

[duoming@zju.edu.cn]

On Sat, 03 Jan 2026 14:55:54 -0500, James Bottomley wrote:
> > > > diff --git a/drivers/scsi/ppa.c b/drivers/scsi/ppa.c
> > > > index ea682f3044b..8da2a78ebac 100644
> > > > --- a/drivers/scsi/ppa.c
> > > > +++ b/drivers/scsi/ppa.c
> > > > @@ -1136,6 +1136,7 @@ static void ppa_detach(struct parport *pb)
> > > >  	ppa_struct *dev;
> > > >  	list_for_each_entry(dev, &ppa_hosts, list) {
> > > >  		if (dev->dev->port == pb) {
> > > > +			disable_delayed_work_sync(&dev->ppa_tq);
> > > >  			list_del_init(&dev->list);
> > > >  			scsi_remove_host(dev->host);
> > > >  			scsi_host_put(dev->host);
> > > 
> > > This fix looks bogus: if there's an active workqueue on ppa it's
> > > because there's an outstanding command and it's emulating polling. 
> > > If you stop the polling by disabling the workqueue, the command
> > > will never return and the host will never get freed, so this will
> > > leak resources, won't it?
> > 
> > I believe that disabling the ppa_tq delayed work will not affect the
> > Scsi_Host release process. The lifetime of the Scsi_Host is managed
> > by tagset_refcnt. The tagset_refcnt is initialized to 1 in
> > scsi_add_host_with_dma() and decreased to 0 in scsi_remove_host().
> > since the delayed work callback ppa_interrupt() does not modify
> > tagset_refcnt in any way, the host could be freed as expected.
> 
> Not if something else holds a reference to the host, which an
> outstanding command does.  That's the point I was making above: as long
> as the command doesn't return, everything is pinned and never gets
> freed (well, possibly until timeout).  You cause that because the work
> queue is only active if a command is outstanding, so when you disable
> the queue in that situation the command remains outstanding and can
> never complete normally.

When a request is sent to a SCSI device, a timer is set for the request.
If it times out, the request is directly terminated and removed from the
queue. The specific function call chain is as follows: 

scsi_queue_rq() -> blk_mq_start_request() -> blk_add_timer() 
-> blk_mq_timeout_work()

/**
 * blk_add_timer - Start timeout timer for a single request
 * @req:	request that is about to start running.
 *
 * Notes:
 *    Each request has its own timer, and as it is added to the queue, we
 *    set up the timer. When the request completes, we cancel the timer.
 */
void blk_add_timer(struct request *req)
{
    ...
	/*
	 * Some LLDs, like scsi, peek at the timeout to prevent a
	 * command from being retried forever.
	 */
    ...
}

static void blk_mq_timeout_work(struct work_struct *work)
{
    ...
    blk_mq_wait_quiesce_done();
    blk_mq_handle_expired();
    blk_queue_exit();
    ...
}

Therefore, this patch will not cause memory leaks or issues where
commands never return.

> > > Also the race condition you identify is one of many tied to an
> > > incorrect ppa_struct lifetime: it should never be free'd before the
> > > host itself is gone because a live host may do a callback which
> > > will get the ppa_struct from hostdata, so if the host is still
> > > alive for any reason when ppa_detach() is called, we'll get the
> > > same problem.
> > 
> > The ppa_struct is properly freed only after ensuring the complete
> > removal of the associated Scsi_Host in ppa_detach(). The detailed
> > sequence is as follows:
> > 
> > ...
> >    scsi_remove_host(dev->host);
> >    scsi_host_put(dev->host); //the host is gone
> 
> The host is not gone if that put is not the final one as it won't be if
> there's an outstanding command pinning the device.

The commands have already been handled by the timeout handler, so it will
not cause issues with the Scsi_Host failing to release properly.

Best regards,
Duoming Zhou