Re: [PATCH V2 02/16] ublk: look up ublk task via its pid in timeout handler

[Ming Lei <ming.lei@redhat.com>]

On Fri, Jul 11, 2025 at 09:20:31AM -0400, Caleb Sander Mateos wrote:
> On Mon, Jul 7, 2025 at 9:18 PM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Look up ublk process via its pid in timeout handler, so we can avoid to
> > touch io->task, because it is fragile to touch task structure.
> >
> > It is fine to kill ublk server process and this way is simpler.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 65daa6ed3a8e..d7b5ee96978a 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -1367,14 +1367,19 @@ static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
> >  static enum blk_eh_timer_return ublk_timeout(struct request *rq)
> >  {
> >         struct ublk_queue *ubq = rq->mq_hctx->driver_data;
> > -       struct ublk_io *io = &ubq->ios[rq->tag];
> > -
> > -       if (ubq->flags & UBLK_F_UNPRIVILEGED_DEV) {
> > -               send_sig(SIGKILL, io->task, 0);
> > -               return BLK_EH_DONE;
> > -       }
> > -
> > -       return BLK_EH_RESET_TIMER;
> > +       struct task_struct *p;
> > +       struct pid *pid;
> > +
> > +       if (!(ubq->flags & UBLK_F_UNPRIVILEGED_DEV))
> > +               return BLK_EH_RESET_TIMER;
> > +
> > +       rcu_read_lock();
> > +       pid = find_vpid(ubq->dev->dev_info.ublksrv_pid);
> 
> It looks like ublksrv_pid is set based on whatever the ublk server
> provides in the UBLK_U_CMD_START_DEV/UBLK_U_CMD_END_USER_RECOVERY
> command. I don't see any validation that this is actually the ublk
> server's PID. So couldn't a buggy/malicious ublk server that doesn't
> provide its own PID cause ublk_timeout() to kill some other process
> and leave the ublk I/O pending forever?

Good catch!

Looks we can store the real ublk server pid in ublk_ch_open(), then validate
the passed `ublksrv_pid` in ublk_ctrl_start_dev(), or even always use
the real ublk server pid stored from  ublk_ch_open().

Will do it in next version.


Thanks,
Ming