[PATCH 0/2] ublk: Allow more than 64 ublk devices

[PATCH 0/2] ublk: Allow more than 64 ublk devices

[Mike Christie]

The following patches were made over Linus's tree. They allow users to
configure the max number of ublk devices. We are currently converting
users from tcmu to ublk so the 64 device limit is too small, because we
have setups that have 512-1k devices.

For the second patch, I've tested up to 4K devices, but was not sure
if we wanted to continue to have an artificical limit or code it so
the limit is based on the code. I did the latter thinking tcmu is
used in some clouds and they will want to convert to ublk like we are,
and they probably have larger limits than we do.

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

[Mike Christie]

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,
+				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) {
+		pr_warn("%s: dev id is too large. Max supported is %d\n",
+			__func__, UBLK_MAX_UBLKS);
+		return -EINVAL;
+	}
+
 	ublk_dump_dev_info(&info);
 
 	ret = mutex_lock_killable(&ublk_ctl_mutex);
-- 
2.34.1

[PATCH 2/2] ublk: Make ublks_max configurable

[Mike Christie]

We are converting tcmu applications to ublk, but have systems with up
to 1k devices. This patch allows us to configure the ublks_max from
userspace with the ublks_max modparam.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/block/ublk_drv.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 18e352f8cd6d..2833a81e05c0 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2940,7 +2940,36 @@ static void __exit ublk_exit(void)
 module_init(ublk_init);
 module_exit(ublk_exit);
 
-module_param(ublks_max, int, 0444);
+static int ublk_set_max_ublks(const char *buf, const struct kernel_param *kp)
+{
+	unsigned int max;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &max);
+	if (ret)
+		return ret;
+
+	if (max > UBLK_MAX_UBLKS) {
+		pr_warn("Invalid ublks_max. Max supported is %d\n",
+			UBLK_MAX_UBLKS);
+		return -EINVAL;
+	}
+
+	ublks_max = max;
+	return ret;
+}
+
+static int ublk_get_max_ublks(char *buf, const struct kernel_param *kp)
+{
+	return sysfs_emit(buf, "%d\n", ublks_max);
+}
+
+static const struct kernel_param_ops ublk_max_ublks_ops = {
+	.set = ublk_set_max_ublks,
+	.get = ublk_get_max_ublks,
+};
+
+module_param_cb(ublks_max, &ublk_max_ublks_ops, NULL, 0644);
 MODULE_PARM_DESC(ublks_max, "max number of ublk devices allowed to add(default: 64)");
 
 MODULE_AUTHOR("Ming Lei <ming.lei@redhat.com>");
-- 
2.34.1

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

[Hannes Reinecke]

On 10/1/23 20:54, 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,
> +				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) {
> +		pr_warn("%s: dev id is too large. Max supported is %d\n",
> +			__func__, UBLK_MAX_UBLKS);
> +		return -EINVAL;
> +	}
> +
>   	ublk_dump_dev_info(&info);
>   
>   	ret = mutex_lock_killable(&ublk_ctl_mutex);

Why don't you do away with ublks_max and switch to dynamic minors once 
UBLK_MAX_UBLKS is reached?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Mike Christie]

On 10/2/23 1:08 AM, Hannes Reinecke wrote:
> On 10/1/23 20:54, 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,
>> +                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) {
>> +        pr_warn("%s: dev id is too large. Max supported is %d\n",
>> +            __func__, UBLK_MAX_UBLKS);
>> +        return -EINVAL;
>> +    }
>> +
>>       ublk_dump_dev_info(&info);
>>         ret = mutex_lock_killable(&ublk_ctl_mutex);
> 
> Why don't you do away with ublks_max and switch to dynamic minors once UBLK_MAX_UBLKS is reached?

My concern with dynamic minors was that userspace was assuming the
dev_id and char dev minor matched and might have been doing something
based on that assumption. If that's not the case, then I think we can
just switch to always use dynamic minors right?

Note that there are 2 cases where userspace can pass in the dev_id
and where the kernel allocates it. For the latter we could use
the dynamic minor for the dev_id right now (swap what we do now).
For the former though, if userspace did want the dev_id==minor
then your suggestion makes the code and interface more complicated
and I'm not sure if it's worth it since we can support up to 1
million devices already.

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

[Ming Lei]

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

Re: [PATCH 2/2] ublk: Make ublks_max configurable

[Ming Lei]

On Sun, Oct 01, 2023 at 01:54:48PM -0500, Mike Christie wrote:
> We are converting tcmu applications to ublk, but have systems with up
> to 1k devices. This patch allows us to configure the ublks_max from
> userspace with the ublks_max modparam.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/block/ublk_drv.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 18e352f8cd6d..2833a81e05c0 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -2940,7 +2940,36 @@ static void __exit ublk_exit(void)
>  module_init(ublk_init);
>  module_exit(ublk_exit);
>  
> -module_param(ublks_max, int, 0444);
> +static int ublk_set_max_ublks(const char *buf, const struct kernel_param *kp)
> +{
> +	unsigned int max;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &max);
> +	if (ret)
> +		return ret;
> +
> +	if (max > UBLK_MAX_UBLKS) {
> +		pr_warn("Invalid ublks_max. Max supported is %d\n",
> +			UBLK_MAX_UBLKS);
> +		return -EINVAL;
> +	}
> +
> +	ublks_max = max;
> +	return ret;

It might be nice to reuse builtin helper:

	return param_set_uint_minmax(buf, kp, 0, UBLK_MAX_UBLKS);

> +}
> +
> +static int ublk_get_max_ublks(char *buf, const struct kernel_param *kp)
> +{
> +	return sysfs_emit(buf, "%d\n", ublks_max);
> +}
> +
> +static const struct kernel_param_ops ublk_max_ublks_ops = {
> +	.set = ublk_set_max_ublks,
> +	.get = ublk_get_max_ublks,

Same with above, '.get    = param_get_int,' could be better.

Otherwise, this patch looks fine.

Thanks,
Ming

Re: [PATCH 2/2] ublk: Make ublks_max configurable

[Mike Christie]

On 10/3/23 10:47 AM, Ming Lei wrote:
> On Sun, Oct 01, 2023 at 01:54:48PM -0500, Mike Christie wrote:
>> We are converting tcmu applications to ublk, but have systems with up
>> to 1k devices. This patch allows us to configure the ublks_max from
>> userspace with the ublks_max modparam.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>  drivers/block/ublk_drv.c | 31 ++++++++++++++++++++++++++++++-
>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
>> index 18e352f8cd6d..2833a81e05c0 100644
>> --- a/drivers/block/ublk_drv.c
>> +++ b/drivers/block/ublk_drv.c
>> @@ -2940,7 +2940,36 @@ static void __exit ublk_exit(void)
>>  module_init(ublk_init);
>>  module_exit(ublk_exit);
>>  
>> -module_param(ublks_max, int, 0444);
>> +static int ublk_set_max_ublks(const char *buf, const struct kernel_param *kp)
>> +{
>> +	unsigned int max;
>> +	int ret;
>> +
>> +	ret = kstrtouint(buf, 10, &max);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (max > UBLK_MAX_UBLKS) {
>> +		pr_warn("Invalid ublks_max. Max supported is %d\n",
>> +			UBLK_MAX_UBLKS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ublks_max = max;
>> +	return ret;
> 
> It might be nice to reuse builtin helper:
> 
> 	return param_set_uint_minmax(buf, kp, 0, UBLK_MAX_UBLKS);
> 

Was looking for something like that but missed it. Will
use.


>> +}
>> +
>> +static int ublk_get_max_ublks(char *buf, const struct kernel_param *kp)
>> +{
>> +	return sysfs_emit(buf, "%d\n", ublks_max);
>> +}
>> +
>> +static const struct kernel_param_ops ublk_max_ublks_ops = {
>> +	.set = ublk_set_max_ublks,
>> +	.get = ublk_get_max_ublks,
> 
> Same with above, '.get    = param_get_int,' could be better.
> 

Will do.

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

[Mike Christie]

On 10/3/23 10:36 AM, Ming Lei wrote:
> 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?

We can use UBLK_MINORS. I just used UBLK_MAX_UBLKS because it was only
a difference of one device and I thought using UBLK_MAX_UBLKS in the
all the checks was more consistent.

But yeah, I can see the opposite where it's more clear to use the
exact limit and will change it.


> 
>> +				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.

I can't drop the first part of the check because header->dev_id is a
u32:

struct ublksrv_ctrl_cmd {
        /* sent to which device, must be valid */
        __u32   dev_id;

and userspace is passing in:

dev_id = U32_MAX

to indicate for the kernel to allocate the dev_id.


The weirdness is that we convert dev_id to a int later:

ret = ublk_alloc_dev_number(ub, header->dev_id);

....

static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)

so the header->dev_id gets converted to a signed int and in
ublk_alloc_dev_number U32_MAX gets turned into -1. There
we check the idx/dev_id more similar to what you suggested above.


If you prefer your test, then I can convert it in ublk_ctrl_add_dev:

int dev_id = header->dev_id;

if (dev_id >= UBLK_MAX_UBLKS)
....


ret = ublk_alloc_dev_number(ub, dev_id);

and then all the code will be similar.


> 
> 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
> 

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

[Mike Christie]

On 10/3/23 11:07 AM, Mike Christie wrote:
> On 10/3/23 10:36 AM, Ming Lei wrote:
>> 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?
> We can use UBLK_MINORS. I just used UBLK_MAX_UBLKS because it was only
> a difference of one device and I thought using UBLK_MAX_UBLKS in the
> all the checks was more consistent.
> 
Ignore this. I misread your comment. Will define UBLK_MAX_UBLKS as UBLK_MINORS.

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

[Ming Lei]

On Tue, Oct 03, 2023 at 11:07:37AM -0500, Mike Christie wrote:
> On 10/3/23 10:36 AM, Ming Lei wrote:
> > 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?
> 
> We can use UBLK_MINORS. I just used UBLK_MAX_UBLKS because it was only
> a difference of one device and I thought using UBLK_MAX_UBLKS in the
> all the checks was more consistent.
> 
> But yeah, I can see the opposite where it's more clear to use the
> exact limit and will change it.
> 
> 
> > 
> >> +				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.
> 
> I can't drop the first part of the check because header->dev_id is a
> u32:

Your are right, let's keep the check.

> 
> struct ublksrv_ctrl_cmd {
>         /* sent to which device, must be valid */
>         __u32   dev_id;
> 
> and userspace is passing in:
> 
> dev_id = U32_MAX
> 
> to indicate for the kernel to allocate the dev_id.
> 
> 
> The weirdness is that we convert dev_id to a int later:
> 
> ret = ublk_alloc_dev_number(ub, header->dev_id);
> 
> ....
> 
> static int ublk_alloc_dev_number(struct ublk_device *ub, int idx)
> 
> so the header->dev_id gets converted to a signed int and in
> ublk_alloc_dev_number U32_MAX gets turned into -1. There
> we check the idx/dev_id more similar to what you suggested above.

The thing is that '-1' means auto-id-allocation, and the .dev_id field
should have been defined as -1 from beginning, but it can't change now.

thanks,
Ming