Re: [PATCH 1/2] ublk: Limit dev_id/ub_number values

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

On Sun, Oct 01, 2023 at 01:54:47PM -0500, Mike Christie wrote:
> The dev_id/ub_number is used for the ublk dev's char device's minor
> number so it has to fit into MINORMASK. This patch adds checks to prevent
> userspace from passing a number that's too large and limits what can be
> allocated by the ublk_index_idr for the case where userspace has the
> kernel allocate the dev_id/ub_number.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/block/ublk_drv.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 630ddfe6657b..18e352f8cd6d 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -470,6 +470,7 @@ static DEFINE_MUTEX(ublk_ctl_mutex);
>   * It can be extended to one per-user limit in future or even controlled
>   * by cgroup.
>   */
> +#define UBLK_MAX_UBLKS (UBLK_MINORS - 1)
>  static unsigned int ublks_max = 64;
>  static unsigned int ublks_added;	/* protected by ublk_ctl_mutex */
>  
> @@ -2026,7 +2027,8 @@ static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
>  		if (err == -ENOSPC)
>  			err = -EEXIST;
>  	} else {
> -		err = idr_alloc(&ublk_index_idr, ub, 0, 0, GFP_NOWAIT);
> +		err = idr_alloc(&ublk_index_idr, ub, 0, UBLK_MAX_UBLKS,

'end' parameter of idr_alloc() is exclusive, so I think UBLK_MAX_UBLKS should
be defined as UBLK_MINORS?

> +				GFP_NOWAIT);
>  	}
>  	spin_unlock(&ublk_idr_lock);
>  
> @@ -2305,6 +2307,12 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
>  		return -EINVAL;
>  	}
>  
> +	if (header->dev_id != U32_MAX && header->dev_id > UBLK_MAX_UBLKS) {

I guess 'if (header->dev_id >= UBLK_MAX_UBLKS)' should be enough.

Otherwise, this patch looks fine.


On Mon, Oct 2, 2023 at 2:08 PM Hannes Reinecke <hare@suse.de> wrote:
...
> Why don't you do away with ublks_max and switch to dynamic minors once
> UBLK_MAX_UBLKS is reached?

The current approach follows nvme cdev(see nvme_cdev_add()), and I don't
see any benefit with dynamic minors, especially MINORBITS is 20, and max
minors is 1M, which should be big enough for any use cases.

BTW, looks nvme_cdev_add() needs similar fix too.

Thanks,
Ming