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

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

[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
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;
 }
 
 static struct blk_mq_hw_ctx_sysfs_entry blk_mq_hw_sysfs_nr_tags = {
-- 
2.20.1

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

[Greg KH]

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?

thanks,

greg k-h

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

[Ming Lei]

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)?


Thanks,
Ming

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

[Greg KH]

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

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

[Ming Lei]

On Thu, Aug 15, 2019 at 02:35:35PM +0200, Greg KH wrote:
> 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?

It depends on machine. The issue is reported on one machine with 896 CPU
cores, when 4K buffer can only hold 841 cores.

> 
> 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" :)

I agree, and this file shouldn't be there, given each CPU will have one
kobject dir under the hctx dir.

We may kill the 'cpu_list' attribute, is there anyone who objects?


Thanks,
Ming

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

[Greg KH]

On Thu, Aug 15, 2019 at 08:43:22PM +0800, Ming Lei wrote:
> On Thu, Aug 15, 2019 at 02:35:35PM +0200, Greg KH wrote:
> > 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?
> 
> It depends on machine. The issue is reported on one machine with 896 CPU
> cores, when 4K buffer can only hold 841 cores.
> 
> > 
> > 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" :)
> 
> I agree, and this file shouldn't be there, given each CPU will have one
> kobject dir under the hctx dir.
> 
> We may kill the 'cpu_list' attribute, is there anyone who objects?

Given that it was never documented, perhaps no one actually uses it :)

greg k-h

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

[Ray, Mark C (Global Solutions Engineering (GSE))]

Hi Ming,

In the customer case, the cpu_list file was not needed.   It was just part of a SAP Hana script to collect all the block device data (similar to sosreport).    So they were just dumping everything, and it picks up the mq-related files.  

I know with IRQs, we have bitmaps/mask, and can represent the list such as "0-27", without listing every CPU.   I'm sure there's lots of options to address this, and getting rid of the cpu_list is one of them.

Best Regards,

Mark Ray
HPE Global Solutions Engineering
mark.ray@hpe.com



-----Original Message-----
From: Ming Lei [mailto:ming.lei@redhat.com] 
Sent: Thursday, August 15, 2019 9:43 PM
To: Greg KH <gregkh@linuxfoundation.org>
Cc: Jens Axboe <axboe@kernel.dk>; linux-block@vger.kernel.org; stable@vger.kernel.org; Ray, Mark C (Global Solutions Engineering (GSE)) <mark.ray@hpe.com>
Subject: Re: [PATCH] blk-mq: avoid sysfs buffer overflow by too many CPU cores

On Thu, Aug 15, 2019 at 02:35:35PM +0200, Greg KH wrote:
> 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?

It depends on machine. The issue is reported on one machine with 896 CPU cores, when 4K buffer can only hold 841 cores.

> 
> 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" :)

I agree, and this file shouldn't be there, given each CPU will have one kobject dir under the hctx dir.

We may kill the 'cpu_list' attribute, is there anyone who objects?


Thanks,
Ming

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

[Ming Lei]

On Thu, Aug 15, 2019 at 11:10:35PM +0000, Ray, Mark C (Global Solutions Engineering (GSE)) wrote:
> Hi Ming,
> 
> In the customer case, the cpu_list file was not needed.   It was just part of a SAP Hana script to collect all the block device data (similar to sosreport).    So they were just dumping everything, and it picks up the mq-related files.  
> 
> I know with IRQs, we have bitmaps/mask, and can represent the list such as "0-27", without listing every CPU.   I'm sure there's lots of options to address this, and getting rid of the cpu_list is one of them.

Indeed, same with several attributes under /sys/devices/system/cpu/,
actually we can use cpumap_print_to_pagebuf() to print the CPUs.

Thanks,
Ming

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

[Greg KH]

On Fri, Aug 16, 2019 at 10:49:35AM +0800, Ming Lei wrote:
> On Thu, Aug 15, 2019 at 11:10:35PM +0000, Ray, Mark C (Global Solutions Engineering (GSE)) wrote:
> > Hi Ming,
> > 
> > In the customer case, the cpu_list file was not needed.   It was just part of a SAP Hana script to collect all the block device data (similar to sosreport).    So they were just dumping everything, and it picks up the mq-related files.  
> > 
> > I know with IRQs, we have bitmaps/mask, and can represent the list such as "0-27", without listing every CPU.   I'm sure there's lots of options to address this, and getting rid of the cpu_list is one of them.
> 
> Indeed, same with several attributes under /sys/devices/system/cpu/,
> actually we can use cpumap_print_to_pagebuf() to print the CPUs.

And that is changing the format of the file, which means it is obvious
no one is using it, so just please delete the thing.

thanks,

greg k-h

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

[Jens Axboe]

On 8/16/19 1:12 AM, Greg KH wrote:
> On Fri, Aug 16, 2019 at 10:49:35AM +0800, Ming Lei wrote:
>> On Thu, Aug 15, 2019 at 11:10:35PM +0000, Ray, Mark C (Global Solutions Engineering (GSE)) wrote:
>>> Hi Ming,
>>>
>>> In the customer case, the cpu_list file was not needed.   It was just part of a SAP Hana script to collect all the block device data (similar to sosreport).    So they were just dumping everything, and it picks up the mq-related files.
>>>
>>> I know with IRQs, we have bitmaps/mask, and can represent the list such as "0-27", without listing every CPU.   I'm sure there's lots of options to address this, and getting rid of the cpu_list is one of them.
>>
>> Indeed, same with several attributes under /sys/devices/system/cpu/,
>> actually we can use cpumap_print_to_pagebuf() to print the CPUs.
> 
> And that is changing the format of the file, which means it is obvious
> no one is using it, so just please delete the thing.

IFF that patch was valid, then yes, it follows that you could delete it.
But that's not a given.

-- 
Jens Axboe

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

[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 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(-)

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 = {
-- 
2.20.1

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

[Jens Axboe]

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.

-- 
Jens Axboe

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

[Chaitanya Kulkarni]

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 ?

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

[Jens Axboe]

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.


-- 
Jens Axboe

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

[Chaitanya Kulkarni]

Okay. 

> Just more noise I think, really wouldn't serve any purpose.
> 
> 
> -- 
> Jens Axboe
> 

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

[Ming Lei]

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

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

[Jens Axboe]

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.

-- 
Jens Axboe

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

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

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

[Jens Axboe]

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?

-- 
Jens Axboe