| Message ID | 20240617194451.435445-2-ushankar@purestorage.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk: support device recovery without I/O queueing | expand |
On Mon, Jun 17, 2024 at 01:44:48PM -0600, Uday Shankar wrote: > Only certain combinations of recovery flags are valid. For example, > setting UBLK_F_USER_RECOVERY_REISSUE without also setting > UBLK_F_USER_RECOVERY is currently silently equivalent to not setting any > recovery flags. Check for such issues and fail add_dev if they are > detected. > > Signed-off-by: Uday Shankar <ushankar@purestorage.com> > --- > drivers/block/ublk_drv.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c > index 4e159948c912..2752a9afe9d4 100644 > --- a/drivers/block/ublk_drv.c > +++ b/drivers/block/ublk_drv.c > @@ -59,6 +59,9 @@ > | UBLK_F_USER_COPY \ > | UBLK_F_ZONED) > > +#define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \ > + | UBLK_F_USER_RECOVERY_REISSUE) > + > /* All UBLK_PARAM_TYPE_* should be included here */ > #define UBLK_PARAM_TYPE_ALL \ > (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \ > @@ -2341,6 +2344,18 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) > else if (!(info.flags & UBLK_F_UNPRIVILEGED_DEV)) > return -EPERM; > > + /* forbid nonsense combinations of recovery flags */ > + switch (info.flags & UBLK_F_ALL_RECOVERY_FLAGS) { > + case 0: > + case UBLK_F_USER_RECOVERY: > + case (UBLK_F_USER_RECOVERY | UBLK_F_USER_RECOVERY_REISSUE): > + break; > + default: > + pr_warn("%s: invalid recovery flags %llx\n", __func__, > + info.flags & UBLK_F_ALL_RECOVERY_FLAGS); > + return -EINVAL; > + } > + It could be cleaner and more readable to check the fail condition only: if ((info.flags & UBLK_F_USER_RECOVERY_REISSUE) && !(info.flags & UBLK_F_USER_RECOVERY)) { ... } Thanks, Ming
On Tue, Jun 18, 2024 at 10:00:08AM +0800, Ming Lei wrote: > > + /* forbid nonsense combinations of recovery flags */ > > + switch (info.flags & UBLK_F_ALL_RECOVERY_FLAGS) { > > + case 0: > > + case UBLK_F_USER_RECOVERY: > > + case (UBLK_F_USER_RECOVERY | UBLK_F_USER_RECOVERY_REISSUE): > > + break; > > + default: > > + pr_warn("%s: invalid recovery flags %llx\n", __func__, > > + info.flags & UBLK_F_ALL_RECOVERY_FLAGS); > > + return -EINVAL; > > + } > > + > > It could be cleaner and more readable to check the fail condition only: > > if ((info.flags & UBLK_F_USER_RECOVERY_REISSUE) && > !(info.flags & UBLK_F_USER_RECOVERY)) { > ... > } Will do. But note that in patch 4 this check gets more complex so we may change it back to a switch statement.
diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c index 4e159948c912..2752a9afe9d4 100644 --- a/drivers/block/ublk_drv.c +++ b/drivers/block/ublk_drv.c @@ -59,6 +59,9 @@ | UBLK_F_USER_COPY \ | UBLK_F_ZONED) +#define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \ + | UBLK_F_USER_RECOVERY_REISSUE) + /* All UBLK_PARAM_TYPE_* should be included here */ #define UBLK_PARAM_TYPE_ALL \ (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \ @@ -2341,6 +2344,18 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd) else if (!(info.flags & UBLK_F_UNPRIVILEGED_DEV)) return -EPERM; + /* forbid nonsense combinations of recovery flags */ + switch (info.flags & UBLK_F_ALL_RECOVERY_FLAGS) { + case 0: + case UBLK_F_USER_RECOVERY: + case (UBLK_F_USER_RECOVERY | UBLK_F_USER_RECOVERY_REISSUE): + break; + default: + pr_warn("%s: invalid recovery flags %llx\n", __func__, + info.flags & UBLK_F_ALL_RECOVERY_FLAGS); + return -EINVAL; + } + /* * unprivileged device can't be trusted, but RECOVERY and * RECOVERY_REISSUE still may hang error handling, so can't
Only certain combinations of recovery flags are valid. For example, setting UBLK_F_USER_RECOVERY_REISSUE without also setting UBLK_F_USER_RECOVERY is currently silently equivalent to not setting any recovery flags. Check for such issues and fail add_dev if they are detected. Signed-off-by: Uday Shankar <ushankar@purestorage.com> --- drivers/block/ublk_drv.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)