| Message ID | 20250304-set_params-v1-1-17b5e0887606@purestorage.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ublk: set_params: properly check if parameters can be applied | expand |
| Context | Check | Description |
|---|---|---|
| shin/vmtest-linus-master-PR | success | PR summary |
| shin/vmtest-linus-master-VM_Test-0 | success | Logs for build-kernel |
On Tue, Mar 04, 2025 at 02:34:26PM -0700, Uday Shankar wrote: > The parameters set by the set_params call are only applied to the block > device in the start_dev call. So if a device has already been started, a > subsequently issued set_params on that device will not have the desired > effect, and should return an error. There is an existing check for this > - set_params fails on devices in the LIVE state. But this check is not > sufficient to cover the recovery case. In this case, the device will be > in the QUIESCED or FAIL_IO states, so set_params will succeed. But this > success is misleading, because the parameters will not be applied, since > the device has already been started (by a previous ublk server). The bit > UB_STATE_USED is set on completion of the start_dev; use it to detect > and fail set_params commands which arrive too late to be applied (after > start_dev). > > Signed-off-by: Uday Shankar <ushankar@purestorage.com> > --- > drivers/block/ublk_drv.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index 2955900ee713c5d8f3cbc2a69f6f6058348e5253..aa34594c76ad02b1480b9ef4a2bd52a095ca6f3f 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -2782,9 +2782,12 @@ static int ublk_ctrl_set_params(struct ublk_device *ub, > if (ph.len > sizeof(struct ublk_params)) > ph.len = sizeof(struct ublk_params); > > - /* parameters can only be changed when device isn't live */ > mutex_lock(&ub->mutex); > - if (ub->dev_info.state == UBLK_S_DEV_LIVE) { > + if (test_bit(UB_STATE_USED, &ub->state)) { > + /* > + * Parameters can only be changed when device hasn't > + * been started yet > + */ > ret = -EACCES; > } else if (copy_from_user(&ub->params, argp, ph.len)) { > ret = -EFAULT; Good catch, Fixes: 0aa73170eba5 ("ublk_drv: add SET_PARAMS/GET_PARAMS control command") Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
On Tue, 04 Mar 2025 14:34:26 -0700, Uday Shankar wrote: > The parameters set by the set_params call are only applied to the block > device in the start_dev call. So if a device has already been started, a > subsequently issued set_params on that device will not have the desired > effect, and should return an error. There is an existing check for this > - set_params fails on devices in the LIVE state. But this check is not > sufficient to cover the recovery case. In this case, the device will be > in the QUIESCED or FAIL_IO states, so set_params will succeed. But this > success is misleading, because the parameters will not be applied, since > the device has already been started (by a previous ublk server). The bit > UB_STATE_USED is set on completion of the start_dev; use it to detect > and fail set_params commands which arrive too late to be applied (after > start_dev). > > [...] Applied, thanks! [1/1] ublk: set_params: properly check if parameters can be applied (no commit info) Best regards,
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 2955900ee713c5d8f3cbc2a69f6f6058348e5253..aa34594c76ad02b1480b9ef4a2bd52a095ca6f3f 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -2782,9 +2782,12 @@ static int ublk_ctrl_set_params(struct ublk_device *ub, if (ph.len > sizeof(struct ublk_params)) ph.len = sizeof(struct ublk_params); - /* parameters can only be changed when device isn't live */ mutex_lock(&ub->mutex); - if (ub->dev_info.state == UBLK_S_DEV_LIVE) { + if (test_bit(UB_STATE_USED, &ub->state)) { + /* + * Parameters can only be changed when device hasn't + * been started yet + */ ret = -EACCES; } else if (copy_from_user(&ub->params, argp, ph.len)) { ret = -EFAULT;
The parameters set by the set_params call are only applied to the block device in the start_dev call. So if a device has already been started, a subsequently issued set_params on that device will not have the desired effect, and should return an error. There is an existing check for this - set_params fails on devices in the LIVE state. But this check is not sufficient to cover the recovery case. In this case, the device will be in the QUIESCED or FAIL_IO states, so set_params will succeed. But this success is misleading, because the parameters will not be applied, since the device has already been started (by a previous ublk server). The bit UB_STATE_USED is set on completion of the start_dev; use it to detect and fail set_params commands which arrive too late to be applied (after start_dev). Signed-off-by: Uday Shankar <ushankar@purestorage.com> --- drivers/block/ublk_drv.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) --- base-commit: 6499457f08ac23a1ad16653b67af7f04ff5dd9e2 change-id: 20250304-set_params-d27f384f2957 Best regards,