| Message ID | 20190816074849.7197-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: remove blk_mq_hw_sysfs_cpus | expand |
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>
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.
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
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.
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
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
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);
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(-)