| Message ID | 20231001185448.48893-2-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ublk: Allow more than 64 ublk devices | expand |
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
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.
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
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 >
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.
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
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);
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(-)