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

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

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