Re: [PATCH] blk-mq: avoid sysfs buffer overflow by too many CPU cores

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Aug 15, 2019 at 08:29:10PM +0800, Ming Lei wrote:
> On Thu, Aug 15, 2019 at 02:24:19PM +0200, Greg KH wrote:
> > On Thu, Aug 15, 2019 at 08:15:18PM +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
> > > one hctx.
> > > 
> > > So use snprintf for avoiding the potential buffer overflow.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Cc: Mark Ray <mark.ray@hpe.com>
> > > 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 | 30 ++++++++++++++++++------------
> > >  1 file changed, 18 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/block/blk-mq-sysfs.c b/block/blk-mq-sysfs.c
> > > index d6e1a9bd7131..e75f41a98415 100644
> > > --- a/block/blk-mq-sysfs.c
> > > +++ b/block/blk-mq-sysfs.c
> > > @@ -164,22 +164,28 @@ 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);
> > >  }
> > >  
> > > +/* avoid overflow by too many CPU cores */
> > >  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;
> > > +	unsigned int cpu = cpumask_first(hctx->cpumask);
> > > +	ssize_t len = snprintf(page, PAGE_SIZE - 1, "%u", cpu);
> > > +	int last_len = len;
> > > +
> > > +	while ((cpu = cpumask_next(cpu, hctx->cpumask)) < nr_cpu_ids) {
> > > +		int cur_len = snprintf(page + len, PAGE_SIZE - 1 - len,
> > > +				       ", %u", cpu);
> > > +		if (cur_len >= PAGE_SIZE - 1 - len) {
> > > +			len -= last_len;
> > > +			len += snprintf(page + len, PAGE_SIZE - 1 - len,
> > > +					"...");
> > > +			break;
> > > +		}
> > > +		len += cur_len;
> > > +		last_len = cur_len;
> > >  	}
> > >  
> > > -	ret += sprintf(ret + page, "\n");
> > > -	return ret;
> > > +	len += snprintf(page + len, PAGE_SIZE - 1 - len, "\n");
> > > +	return len;
> > >  }
> > >
> > 
> > What????
> > 
> > sysfs is "one value per file".  You should NEVER have to care about the
> > size of the sysfs buffer.  If you do, you are doing something wrong.
> > 
> > What excatly are you trying to show in this sysfs file?  I can't seem to
> > find the Documenatation/ABI/ entry for it, am I just missing it because
> > I don't know the filename for it?
> 
> It is /sys/block/$DEV/mq/$N/cpu_list, all CPUs in this hctx($N) will be
> shown via sysfs buffer. The buffer size is one PAGE, how can it hold when
> there are too many CPUs(close to 1K)?

Looks like I only see 1 cpu listed on my machines in those files, what
am I doing wrong?

Also, I don't see cpu_list in any of the documentation files, so I have
no idea what you are trying to have this file show.

And again, "one value per file" is the sysfs rule.  "all cpus in the
system" is not "one value" :)

thanks,

greg k-h