diff mbox series

[1/4] ublk: check recovery flags for validity

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

Commit Message

Uday Shankar June 17, 2024, 7:44 p.m. UTC
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(+)

Comments

Ming Lei June 18, 2024, 2 a.m. UTC | #1
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
Uday Shankar July 23, 2024, 7:32 p.m. UTC | #2
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 mbox series

Patch

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