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@xxxxxxxxxx>
>> ---
>> 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.
