Message ID | 20191102080215.20223-1-ming.lei@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | blk-mq: avoid sysfs buffer overflow by too many CPU cores | expand |
On 11/2/19 2:02 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 of > hctx via /sys/block/$DEV/mq/$N/cpu_list. > > So use snprintf for avoiding the potential buffer overflow. > > This version doesn't change the attribute format, and simply stop > to show CPU number if the buffer is to be overflow. Applied, thanks.
Ming, On 11/02/2019 01:02 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 of > hctx via/sys/block/$DEV/mq/$N/cpu_list. > > So use snprintf for avoiding the potential buffer overflow. > > This version doesn't change the attribute format, and simply stop > to show CPU number if the buffer is to be overflow. Does it make sense to also add a print or WARN_ON in case of overflow ?
On 11/2/19 6:25 PM, Chaitanya Kulkarni wrote: > Ming, > > On 11/02/2019 01:02 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 of >> hctx via/sys/block/$DEV/mq/$N/cpu_list. >> >> So use snprintf for avoiding the potential buffer overflow. >> >> This version doesn't change the attribute format, and simply stop >> to show CPU number if the buffer is to be overflow. > > Does it make sense to also add a print or WARN_ON in case of overflow ? Just more noise I think, really wouldn't serve any purpose.
Okay. > Just more noise I think, really wouldn't serve any purpose. > > > -- > Jens Axboe >
On Sun, Nov 3, 2019 at 8:28 AM Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> wrote: > > Ming, > > On 11/02/2019 01:02 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 of > > hctx via/sys/block/$DEV/mq/$N/cpu_list. > > > > So use snprintf for avoiding the potential buffer overflow. > > > > This version doesn't change the attribute format, and simply stop > > to show CPU number if the buffer is to be overflow. > > Does it make sense to also add a print or WARN_ON in case of overflow ? Yes, it does, could you cook a patch for that? Thanks, Ming Lei
On 11/3/19 4:57 PM, Ming Lei wrote: > On Sun, Nov 3, 2019 at 8:28 AM Chaitanya Kulkarni > <Chaitanya.Kulkarni@wdc.com> wrote: >> >> Ming, >> >> On 11/02/2019 01:02 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 of >>> hctx via/sys/block/$DEV/mq/$N/cpu_list. >>> >>> So use snprintf for avoiding the potential buffer overflow. >>> >>> This version doesn't change the attribute format, and simply stop >>> to show CPU number if the buffer is to be overflow. >> >> Does it make sense to also add a print or WARN_ON in case of overflow ? > > Yes, it does, could you cook a patch for that? No it doesn't. The WARN_ON brings absolutely nothing. If you're using a script, it gets the same values out and doesn't see the warning. If it's a human cat'ing it, they will probably already realize that we're missing CPUs. Or maybe not even see the warning. It's useless. We should either make this seqfile, or just kill the file. Those are the only two options that make any sense.
On 11/4/19 2:56 AM, Jens Axboe wrote: > On 11/3/19 4:57 PM, Ming Lei wrote: >> On Sun, Nov 3, 2019 at 8:28 AM Chaitanya Kulkarni >> <Chaitanya.Kulkarni@wdc.com> wrote: >>> >>> Ming, >>> >>> On 11/02/2019 01:02 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 of >>>> hctx via/sys/block/$DEV/mq/$N/cpu_list. >>>> >>>> So use snprintf for avoiding the potential buffer overflow. >>>> >>>> This version doesn't change the attribute format, and simply stop >>>> to show CPU number if the buffer is to be overflow. >>> >>> Does it make sense to also add a print or WARN_ON in case of overflow ? >> >> Yes, it does, could you cook a patch for that? > > No it doesn't. The WARN_ON brings absolutely nothing. If you're using > a script, it gets the same values out and doesn't see the warning. If > it's a human cat'ing it, they will probably already realize that > we're missing CPUs. Or maybe not even see the warning. It's useless. > > We should either make this seqfile, or just kill the file. Those are > the only two options that make any sense. > I'd rather retain that file; it proved really useful when debugging interrupt affinity issues. Cheers, Hannes
On 11/3/19 11:57 PM, Hannes Reinecke wrote: > On 11/4/19 2:56 AM, Jens Axboe wrote: >> On 11/3/19 4:57 PM, Ming Lei wrote: >>> On Sun, Nov 3, 2019 at 8:28 AM Chaitanya Kulkarni >>> <Chaitanya.Kulkarni@wdc.com> wrote: >>>> >>>> Ming, >>>> >>>> On 11/02/2019 01:02 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 of >>>>> hctx via/sys/block/$DEV/mq/$N/cpu_list. >>>>> >>>>> So use snprintf for avoiding the potential buffer overflow. >>>>> >>>>> This version doesn't change the attribute format, and simply stop >>>>> to show CPU number if the buffer is to be overflow. >>>> >>>> Does it make sense to also add a print or WARN_ON in case of overflow ? >>> >>> Yes, it does, could you cook a patch for that? >> >> No it doesn't. The WARN_ON brings absolutely nothing. If you're using >> a script, it gets the same values out and doesn't see the warning. If >> it's a human cat'ing it, they will probably already realize that >> we're missing CPUs. Or maybe not even see the warning. It's useless. >> >> We should either make this seqfile, or just kill the file. Those are >> the only two options that make any sense. >> > I'd rather retain that file; it proved really useful when debugging > interrupt affinity issues. Care to take a look at converting to seq_file?
diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c index a0d3ce30fa08..68996ef1d339 100644 --- a/block/blk-mq-sysfs.c +++ b/block/blk-mq-sysfs.c @@ -166,20 +166,25 @@ static ssize_t blk_mq_hw_sysfs_nr_reserved_tags_show(struct blk_mq_hw_ctx *hctx, static ssize_t blk_mq_hw_sysfs_cpus_show(struct blk_mq_hw_ctx *hctx, char *page) { + const size_t size = PAGE_SIZE - 1; unsigned int i, first = 1; - ssize_t ret = 0; + int ret = 0, pos = 0; for_each_cpu(i, hctx->cpumask) { if (first) - ret += sprintf(ret + page, "%u", i); + ret = snprintf(pos + page, size - pos, "%u", i); else - ret += sprintf(ret + page, ", %u", i); + ret = snprintf(pos + page, size - pos, ", %u", i); + + if (ret >= size - pos) + break; first = 0; + pos += ret; } - ret += sprintf(ret + page, "\n"); - return ret; + ret = snprintf(pos + page, size - pos, "\n"); + return pos + ret; } static struct blk_mq_hw_ctx_sysfs_entry blk_mq_hw_sysfs_nr_tags = {
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 of hctx via /sys/block/$DEV/mq/$N/cpu_list. So use snprintf for avoiding the potential buffer overflow. This version doesn't change the attribute format, and simply stop to show CPU number if the buffer is to be overflow. Cc: stable@vger.kernel.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 | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)