[PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[Ming Lei]

It is reported that sysfs buffer overflow can be triggered in case
of too many CPU cores(>841 on 4K PAGE_SIZE) when showing CPUs in
blk_mq_hw_sysfs_cpus_show().

This info isn't useful, given users may retrieve the CPU list
from sw queue entries under same kobject dir, so far not see
any active users.

So remove the entry as suggested by Greg.

Cc: stable@vger.kernel.org
Cc: Mark Ray <mark.ray@hpe.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Fixes: 676141e48af7("blk-mq: don't dump CPU -> hw queue map on driver load")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq-sysfs.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
index d6e1a9bd7131..e0b97c22726c 100644
--- a/block/blk-mq-sysfs.c
+++ b/block/blk-mq-sysfs.c
@@ -164,24 +164,6 @@ static ssize_t blk_mq_hw_sysfs_nr_reserved_tags_show(struct blk_mq_hw_ctx *hctx,
 	return sprintf(page, "%u\n", hctx->tags->nr_reserved_tags);
 }
 
-static ssize_t blk_mq_hw_sysfs_cpus_show(struct blk_mq_hw_ctx *hctx, char *page)
-{
-	unsigned int i, first = 1;
-	ssize_t ret = 0;
-
-	for_each_cpu(i, hctx->cpumask) {
-		if (first)
-			ret += sprintf(ret + page, "%u", i);
-		else
-			ret += sprintf(ret + page, ", %u", i);
-
-		first = 0;
-	}
-
-	ret += sprintf(ret + page, "\n");
-	return ret;
-}
-
 static struct blk_mq_hw_ctx_sysfs_entry blk_mq_hw_sysfs_nr_tags = {
 	.attr = {.name = "nr_tags", .mode = 0444 },
 	.show = blk_mq_hw_sysfs_nr_tags_show,
@@ -190,15 +172,10 @@ static struct blk_mq_hw_ctx_sysfs_entry blk_mq_hw_sysfs_nr_reserved_tags = {
 	.attr = {.name = "nr_reserved_tags", .mode = 0444 },
 	.show = blk_mq_hw_sysfs_nr_reserved_tags_show,
 };
-static struct blk_mq_hw_ctx_sysfs_entry blk_mq_hw_sysfs_cpus = {
-	.attr = {.name = "cpu_list", .mode = 0444 },
-	.show = blk_mq_hw_sysfs_cpus_show,
-};
 
 static struct attribute *default_hw_ctx_attrs[] = {
 	&blk_mq_hw_sysfs_nr_tags.attr,
 	&blk_mq_hw_sysfs_nr_reserved_tags.attr,
-	&blk_mq_hw_sysfs_cpus.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(default_hw_ctx);
-- 
2.20.1

Re: [PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[Greg KH]

On Fri, Aug 16, 2019 at 03:48:49PM +0800, Ming Lei wrote:
> It is reported that sysfs buffer overflow can be triggered in case
> of too many CPU cores(>841 on 4K PAGE_SIZE) when showing CPUs in
> blk_mq_hw_sysfs_cpus_show().
> 
> This info isn't useful, given users may retrieve the CPU list
> from sw queue entries under same kobject dir, so far not see
> any active users.
> 
> So remove the entry as suggested by Greg.
> 
> Cc: stable@vger.kernel.org
> Cc: Mark Ray <mark.ray@hpe.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Fixes: 676141e48af7("blk-mq: don't dump CPU -> hw queue map on driver load")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-mq-sysfs.c | 23 -----------------------
>  1 file changed, 23 deletions(-)


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[Jens Axboe]

On 8/16/19 1:48 AM, Ming Lei wrote:
> It is reported that sysfs buffer overflow can be triggered in case
> of too many CPU cores(>841 on 4K PAGE_SIZE) when showing CPUs in
> blk_mq_hw_sysfs_cpus_show().
> 
> This info isn't useful, given users may retrieve the CPU list
> from sw queue entries under same kobject dir, so far not see
> any active users.
> 
> So remove the entry as suggested by Greg.

I think that's a bit frivolous, there could very well be scripts or
apps that use it. Let's just fix the overflow.

-- 
Jens Axboe

Re: [PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[Greg KH]

On Fri, Aug 16, 2019 at 08:20:42AM -0600, Jens Axboe wrote:
> On 8/16/19 1:48 AM, Ming Lei wrote:
> > It is reported that sysfs buffer overflow can be triggered in case
> > of too many CPU cores(>841 on 4K PAGE_SIZE) when showing CPUs in
> > blk_mq_hw_sysfs_cpus_show().
> > 
> > This info isn't useful, given users may retrieve the CPU list
> > from sw queue entries under same kobject dir, so far not see
> > any active users.
> > 
> > So remove the entry as suggested by Greg.
> 
> I think that's a bit frivolous, there could very well be scripts or
> apps that use it. Let's just fix the overflow.

As no one really knows what the format is (and the patch to fix the
overflow changes the format of the file), I would say that it needs to
just be dropped as it is not an example of what you should be doing in
sysfs.

thanks,

greg k-h

Re: [PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[Jens Axboe]

On 8/16/19 8:54 AM, Greg KH wrote:
> On Fri, Aug 16, 2019 at 08:20:42AM -0600, Jens Axboe wrote:
>> On 8/16/19 1:48 AM, Ming Lei wrote:
>>> It is reported that sysfs buffer overflow can be triggered in case
>>> of too many CPU cores(>841 on 4K PAGE_SIZE) when showing CPUs in
>>> blk_mq_hw_sysfs_cpus_show().
>>>
>>> This info isn't useful, given users may retrieve the CPU list
>>> from sw queue entries under same kobject dir, so far not see
>>> any active users.
>>>
>>> So remove the entry as suggested by Greg.
>>
>> I think that's a bit frivolous, there could very well be scripts or
>> apps that use it. Let's just fix the overflow.
> 
> As no one really knows what the format is (and the patch to fix the
> overflow changes the format of the file), I would say that it needs to
> just be dropped as it is not an example of what you should be doing in
> sysfs.

It's a list of CPUs, I think the format is quite self explanatory?

But in any case, I'm not 100% opposed to removing it, it's just not
one of those things that should be done on a whim.

-- 
Jens Axboe

Re: [PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[Greg KH]

On Fri, Aug 16, 2019 at 09:12:33AM -0600, Jens Axboe wrote:
> On 8/16/19 8:54 AM, Greg KH wrote:
> > On Fri, Aug 16, 2019 at 08:20:42AM -0600, Jens Axboe wrote:
> >> On 8/16/19 1:48 AM, Ming Lei wrote:
> >>> It is reported that sysfs buffer overflow can be triggered in case
> >>> of too many CPU cores(>841 on 4K PAGE_SIZE) when showing CPUs in
> >>> blk_mq_hw_sysfs_cpus_show().
> >>>
> >>> This info isn't useful, given users may retrieve the CPU list
> >>> from sw queue entries under same kobject dir, so far not see
> >>> any active users.
> >>>
> >>> So remove the entry as suggested by Greg.
> >>
> >> I think that's a bit frivolous, there could very well be scripts or
> >> apps that use it. Let's just fix the overflow.
> > 
> > As no one really knows what the format is (and the patch to fix the
> > overflow changes the format of the file), I would say that it needs to
> > just be dropped as it is not an example of what you should be doing in
> > sysfs.
> 
> It's a list of CPUs, I think the format is quite self explanatory?

I'm not disagreeing, but the patch to fix the length changes the
formatting to be different.  So if you were needing to parse that file,
it now just broke the parser.

Which is why sysfs is to be one-value-per-file :)

> But in any case, I'm not 100% opposed to removing it, it's just not
> one of those things that should be done on a whim.

If the format of the file is going to change, I would argue that the
filename should change as well, so that it's obvious what is happening
here.  This is how we got in trouble with /proc files so many times...

thanks,

greg k-h

Re: [PATCH] blk-mq: remove blk_mq_hw_sysfs_cpus

[Ming Lei]

On Fri, Aug 16, 2019 at 08:20:42AM -0600, Jens Axboe wrote:
> On 8/16/19 1:48 AM, Ming Lei wrote:
> > It is reported that sysfs buffer overflow can be triggered in case
> > of too many CPU cores(>841 on 4K PAGE_SIZE) when showing CPUs in
> > blk_mq_hw_sysfs_cpus_show().
> > 
> > This info isn't useful, given users may retrieve the CPU list
> > from sw queue entries under same kobject dir, so far not see
> > any active users.
> > 
> > So remove the entry as suggested by Greg.
> 
> I think that's a bit frivolous, there could very well be scripts or
> apps that use it. Let's just fix the overflow.

I am fine with either way.

There are two fixes:

1) without format change:
https://lore.kernel.org/linux-block/c5b1b6f3-d461-9379-7e5c-6c6bee6a7bd5@kernel.dk/T/#t

2) format change:
https://lore.kernel.org/linux-block/20190816070948.GD1368@kroah.com/T/#t

If either one isn't fine, please let me know, and I will cook new one for you.


Thanks,
Ming