Re: [PATCH] ublk: reset kernel-owned dev_info fields in ublk_ctrl_add_dev()

On Sun, Jul 26, 2026 at 7:50 AM Ming Lei <[email protected]> wrote:
>
> ublk_ctrl_add_dev() memcpy()s the userspace ublksrv_ctrl_dev_info into
> ub->dev_info and then fixes up the fields the driver owns, but misses
> ->state and ->ublksrv_pid.
>
> A device added with ->state = UBLK_S_DEV_LIVE passes the
> "->state != UBLK_S_DEV_DEAD" test that ublk_stop_dev_unlocked() uses as its
> proxy for "a disk is attached", while ->ub_disk is still NULL, so DEL_DEV
> right after ADD_DEV oopses in del_gendisk().  UBLK_S_DEV_QUIESCED plus
> UBLK_F_USER_RECOVERY dies one step earlier, in ublk_force_abort_dev().  A
> poisoned ->state also gets START_USER_RECOVERY and the char device
> read/write path onto a device that was never started, and wedges START_DEV
> at -EEXIST.  A poisoned ->ublksrv_pid just makes GET_DEV_INFO report an
> unrelated task as the ublk server.
>
> Reset both after the memcpy(), as ublk_detach_disk() does.  Userspace only
> ever reads these back, so correcting them silently breaks nothing.
>
> ADD_DEV has copied ->state in unsanitized since ublk was merged, but back
> then it was harmless: the gendisk was allocated during ADD_DEV, and both
> teardown and the START_DEV -EEXIST check keyed off disk_live() rather than
> ->state.  The oops became reachable once the disk allocation moved to
> START_DEV and those checks switched to ->state.
>
> Fixes: 6d9e6dfdf3b2 ("ublk: defer disk allocation")
> Cc: [email protected]
> Signed-off-by: Ming Lei <[email protected]>

Reviewed-by: Caleb Sander Mateos <[email protected]>

> ---
>  drivers/block/ublk_drv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 4ca6ec738c93..2a22f9dc1f2f 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -4764,6 +4764,15 @@ static int ublk_ctrl_add_dev(const struct ublksrv_ctrl_cmd *header)
>         /* update device id */
>         ub->dev_info.dev_id = ub->ub_number;
>
> +       /*
> +        * ->state and ->ublksrv_pid are owned by the driver and only read back
> +        * by userspace, but they come from the copied-in dev_info, so reset
> +        * them. Otherwise a device added with ->state != DEAD looks live while
> +        * ->ub_disk is still NULL.
> +        */
> +       ub->dev_info.state = UBLK_S_DEV_DEAD;
> +       ub->dev_info.ublksrv_pid = -1;
> +
>         /*
>          * 64bit flags will be copied back to userspace as feature
>          * negotiation result, so have to clear flags which driver
> --
> 2.55.0
>