Re: [PATCH V3 01/17] ublk: validate ublk server pid

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

On Tue, Jul 15, 2025 at 11:48:12AM -0400, Caleb Sander Mateos wrote:
> On Tue, Jul 15, 2025 at 11:42 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Tue, Jul 15, 2025 at 10:50:39AM -0400, Caleb Sander Mateos wrote:
> > > On Sun, Jul 13, 2025 at 10:34 AM Ming Lei <ming.lei@redhat.com> wrote:
> > > >
> > > > ublk server pid(the `tgid` of the process opening the ublk device) is stored
> > > > in `ublk_device->ublksrv_tgid`. This `tgid` is then checked against the
> > > > `ublksrv_pid` in `ublk_ctrl_start_dev` and `ublk_ctrl_end_recovery`.
> > > >
> > > > This ensures that correct ublk server pid is stored in device info.
> > > >
> > > > Fixes: 71f28f3136af ("ublk_drv: add io_uring based userspace block driver")
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >  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 a1a700c7e67a..2b894de29823 100644
> > > > --- a/drivers/block/ublk_drv.c
> > > > +++ b/drivers/block/ublk_drv.c
> > > > @@ -237,6 +237,7 @@ struct ublk_device {
> > > >         unsigned int            nr_privileged_daemon;
> > > >         struct mutex cancel_mutex;
> > > >         bool canceling;
> > > > +       pid_t   ublksrv_tgid;
> > > >  };
> > > >
> > > >  /* header of ublk_params */
> > > > @@ -1528,6 +1529,7 @@ static int ublk_ch_open(struct inode *inode, struct file *filp)
> > > >         if (test_and_set_bit(UB_STATE_OPEN, &ub->state))
> > > >                 return -EBUSY;
> > > >         filp->private_data = ub;
> > > > +       ub->ublksrv_tgid = current->tgid;
> > > >         return 0;
> > > >  }
> > > >
> > > > @@ -1542,6 +1544,7 @@ static void ublk_reset_ch_dev(struct ublk_device *ub)
> > > >         ub->mm = NULL;
> > > >         ub->nr_queues_ready = 0;
> > > >         ub->nr_privileged_daemon = 0;
> > > > +       ub->ublksrv_tgid = -1;
> > >
> > > Should this be reset to 0? The next patch checks whether ublksrv_tgid
> > > is 0 in ublk_timeout().
> >
> > No, swapper pid is 0.
> >
> > The check in next patch just tries to double check if ublk char device
> > is opened.
> >
> > > Also, the accesses to it should probably be
> > > using {READ,WRITE}_ONCE() since ublk server open/close can happen
> > > concurrently with ublk I/O timeout handling.
> >
> > ublk_abort_queue() is called in ublk_ch_release(), and any inflight request
> > is either requeued or failed, so ublk I/O timeout handling won't happen
> > concurrently with ublk char open()/close().
> 
> Thanks for explaining. If the ublk server closing the char device
> ensures there are no in-flight requests, does that make the
> ublksrv_tgid check in ublk_timeout() unnecessary?

Yes, the check should have been WARN_ON_ONCE().

Thanks,
Ming