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?
> + 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@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
