On Mon, Jul 7, 2025 at 9:18 PM Ming Lei <ming.lei@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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?
Best,
Caleb
> + p = pid_task(pid, PIDTYPE_PID);
> + if (p)
> + send_sig(SIGKILL, p, 0);
> + rcu_read_unlock();
> + return BLK_EH_DONE;
> }
>
> static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq,
> --
> 2.47.0
>
