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@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,
>
> '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@xxxxxxx> 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
>
