Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
@ 2020-09-14  2:48 Xiongfeng Wang
  2020-09-16 17:00 ` Borislav Petkov
  0 siblings, 1 reply; 8+ messages in thread
From: Xiongfeng Wang @ 2020-09-14  2:48 UTC (permalink / raw)
  To: bp, mchehab, tony.luck; +Cc: linux-edac, linux-kernel, wangxiongfeng2

Reading those sysfs entries gives:

  [root@localhost /]# cat /sys/devices/system/edac/mc/mc0/max_location
  memory 3 [root@localhost /]# cat /sys/devices/system/edac/mc/mc0/dimm0/dimm_location
  memory 0 [root@localhost /]#

Add newlines after the value it prints for better readability.

Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 drivers/edac/edac_mc_sysfs.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 4e6aca5..bf0e075 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -474,8 +474,12 @@ static ssize_t dimmdev_location_show(struct device *dev,
 				     struct device_attribute *mattr, char *data)
 {
 	struct dimm_info *dimm = to_dimm(dev);
+	ssize_t count;
 
-	return edac_dimm_info_location(dimm, data, PAGE_SIZE);
+	count = edac_dimm_info_location(dimm, data, PAGE_SIZE);
+	count += snprintf(data + count, PAGE_SIZE - count, "\n");
+
+	return count;
 }
 
 static ssize_t dimmdev_label_show(struct device *dev,
@@ -813,15 +817,21 @@ static ssize_t mci_max_location_show(struct device *dev,
 				     char *data)
 {
 	struct mem_ctl_info *mci = to_mci(dev);
-	int i;
+	int i, n;
 	char *p = data;
+	unsigned int len = PAGE_SIZE;
 
 	for (i = 0; i < mci->n_layers; i++) {
-		p += sprintf(p, "%s %d ",
+		n = snprintf(p, len, "%s %d ",
 			     edac_layer_name[mci->layers[i].type],
 			     mci->layers[i].size - 1);
+		p += n;
+		len -= n;
+		if (!len)
+			goto out;
 	}
-
+	p += snprintf(p, len, "\n");
+out:
 	return p - data;
 }
 
-- 
1.7.12.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
  2020-09-14  2:48 [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location Xiongfeng Wang
@ 2020-09-16 17:00 ` Borislav Petkov
  2020-09-17 11:38   ` Xiongfeng Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2020-09-16 17:00 UTC (permalink / raw)
  To: Xiongfeng Wang; +Cc: mchehab, tony.luck, linux-edac, linux-kernel

On Mon, Sep 14, 2020 at 10:48:54AM +0800, Xiongfeng Wang wrote:
> @@ -813,15 +817,21 @@ static ssize_t mci_max_location_show(struct device *dev,
>  				     char *data)
>  {
>  	struct mem_ctl_info *mci = to_mci(dev);
> -	int i;
> +	int i, n;
>  	char *p = data;
> +	unsigned int len = PAGE_SIZE;
>  
>  	for (i = 0; i < mci->n_layers; i++) {
> -		p += sprintf(p, "%s %d ",
> +		n = snprintf(p, len, "%s %d ",
>  			     edac_layer_name[mci->layers[i].type],
>  			     mci->layers[i].size - 1);
> +		p += n;
> +		len -= n;

What happens if that subtraction causes len to wrap around and become a
huge positive unsigned integer?

> +		if (!len)

Would that test still work?

IOW, I did this to your patch ontop. Note that I've moved the "p"
pointer incrementation after the length check so that the pointer
doesn't overflow too:

---
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index bf0e075fb635..fa0551c81e63 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -817,19 +817,22 @@ static ssize_t mci_max_location_show(struct device *dev,
 				     char *data)
 {
 	struct mem_ctl_info *mci = to_mci(dev);
-	int i, n;
+	int len = PAGE_SIZE;
 	char *p = data;
-	unsigned int len = PAGE_SIZE;
+	int i, n;
 
 	for (i = 0; i < mci->n_layers; i++) {
 		n = snprintf(p, len, "%s %d ",
 			     edac_layer_name[mci->layers[i].type],
 			     mci->layers[i].size - 1);
-		p += n;
+
 		len -= n;
-		if (!len)
+		if (len < 0)
 			goto out;
+
+		p += n;
 	}
+
 	p += snprintf(p, len, "\n");
 out:
 	return p - data;

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
  2020-09-16 17:00 ` Borislav Petkov
@ 2020-09-17 11:38   ` Xiongfeng Wang
  2020-09-17 15:28     ` Joe Perches
  2020-09-17 16:25     ` Borislav Petkov
  0 siblings, 2 replies; 8+ messages in thread
From: Xiongfeng Wang @ 2020-09-17 11:38 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: mchehab, tony.luck, linux-edac, linux-kernel

Hi ,

On 2020/9/17 1:00, Borislav Petkov wrote:
> On Mon, Sep 14, 2020 at 10:48:54AM +0800, Xiongfeng Wang wrote:
>> @@ -813,15 +817,21 @@ static ssize_t mci_max_location_show(struct device *dev,
>>  				     char *data)
>>  {
>>  	struct mem_ctl_info *mci = to_mci(dev);
>> -	int i;
>> +	int i, n;
>>  	char *p = data;
>> +	unsigned int len = PAGE_SIZE;
>>  
>>  	for (i = 0; i < mci->n_layers; i++) {
>> -		p += sprintf(p, "%s %d ",
>> +		n = snprintf(p, len, "%s %d ",
>>  			     edac_layer_name[mci->layers[i].type],
>>  			     mci->layers[i].size - 1);
>> +		p += n;
>> +		len -= n;
> 
> What happens if that subtraction causes len to wrap around and become a
> huge positive unsigned integer?
> 
>> +		if (!len)
> 
> Would that test still work?

I am not sure if snprintf will return a value larger than its second input
paramter 'size'. But we can also check if 'len' is less than 0. It's better.

> 
> IOW, I did this to your patch ontop. Note that I've moved the "p"
> pointer incrementation after the length check so that the pointer
> doesn't overflow too:

Thanks. I will add it in the next version.

> 
> ---
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index bf0e075fb635..fa0551c81e63 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -817,19 +817,22 @@ static ssize_t mci_max_location_show(struct device *dev,
>  				     char *data)
>  {
>  	struct mem_ctl_info *mci = to_mci(dev);
> -	int i, n;
> +	int len = PAGE_SIZE;
>  	char *p = data;
> -	unsigned int len = PAGE_SIZE;
> +	int i, n;
>  
>  	for (i = 0; i < mci->n_layers; i++) {
>  		n = snprintf(p, len, "%s %d ",
>  			     edac_layer_name[mci->layers[i].type],
>  			     mci->layers[i].size - 1);
> -		p += n;
> +
>  		len -= n;
> -		if (!len)
> +		if (len < 0)

Not sure whether we need to check 'len' equals to 0.
if (len <= 0) ?


>  			goto out;
> +
> +		p += n;
>  	}
> +
>  	p += snprintf(p, len, "\n");
>  out:
>  	return p - data;
> 

Thanks,
XIongfeng

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
  2020-09-17 11:38   ` Xiongfeng Wang
@ 2020-09-17 15:28     ` Joe Perches
  2020-09-17 16:25     ` Borislav Petkov
  1 sibling, 0 replies; 8+ messages in thread
From: Joe Perches @ 2020-09-17 15:28 UTC (permalink / raw)
  To: Xiongfeng Wang, Borislav Petkov
  Cc: mchehab, tony.luck, linux-edac, linux-kernel

On Thu, 2020-09-17 at 19:38 +0800, Xiongfeng Wang wrote:
> On 2020/9/17 1:00, Borislav Petkov wrote:
> > On Mon, Sep 14, 2020 at 10:48:54AM +0800, Xiongfeng Wang wrote:
> > > @@ -813,15 +817,21 @@ static ssize_t mci_max_location_show(struct device *dev,
> > >  				     char *data)
> > >  {
> > >  	struct mem_ctl_info *mci = to_mci(dev);
> > > -	int i;
> > > +	int i, n;
> > >  	char *p = data;
> > > +	unsigned int len = PAGE_SIZE;
> > >  
> > >  	for (i = 0; i < mci->n_layers; i++) {
> > > -		p += sprintf(p, "%s %d ",
> > > +		n = snprintf(p, len, "%s %d ",
> > >  			     edac_layer_name[mci->layers[i].type],
> > >  			     mci->layers[i].size - 1);
> > > +		p += n;
> > > +		len -= n;
> > 
> > What happens if that subtraction causes len to wrap around and become a
> > huge positive unsigned integer?

If you're really concerned about wrapping, use scnprintf.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
  2020-09-17 11:38   ` Xiongfeng Wang
  2020-09-17 15:28     ` Joe Perches
@ 2020-09-17 16:25     ` Borislav Petkov
  2020-09-18  2:37       ` Xiongfeng Wang
  1 sibling, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2020-09-17 16:25 UTC (permalink / raw)
  To: Xiongfeng Wang; +Cc: mchehab, tony.luck, linux-edac, linux-kernel

On Thu, Sep 17, 2020 at 07:38:57PM +0800, Xiongfeng Wang wrote:
> I am not sure if snprintf will return a value larger than its second input
> paramter 'size'.

The comment over snprintf() says

 * The return value is the number of characters which would be
 * generated for the given input, excluding the trailing null,
 * as per ISO C99.  If the return is greater than or equal to
 * @size, the resulting string is truncated.

And let's try it, see diff at the end. Now look what that produces:

[    2.594796] kernel_init: len: 16, str: [A lo]

it returns 16 for len even though the buffer is 5 chars long. So in our
patch, we'd increment by 16 which would be wrong.

Now let's use scnprintf():

[    2.700142] kernel_init: len: 4, str: [A lo]

Much better. Lemme do that.

> Not sure whether we need to check 'len' equals to 0.
> if (len <= 0) ?

Yeah, lemme fix that too, so we have now incrementally ontop:

---
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index fa0551c81e63..c56e0004b39e 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -822,18 +822,17 @@ static ssize_t mci_max_location_show(struct device *dev,
 	int i, n;
 
 	for (i = 0; i < mci->n_layers; i++) {
-		n = snprintf(p, len, "%s %d ",
-			     edac_layer_name[mci->layers[i].type],
-			     mci->layers[i].size - 1);
-
+		n = scnprintf(p, len, "%s %d ",
+			      edac_layer_name[mci->layers[i].type],
+			      mci->layers[i].size - 1);
 		len -= n;
-		if (len < 0)
+		if (len <= 0)
 			goto out;
 
 		p += n;
 	}
 
-	p += snprintf(p, len, "\n");
+	p += scnprintf(p, len, "\n");
 out:
 	return p - data;
 }
---

Test diff:

---
diff --git a/init/main.c b/init/main.c
index ae78fb68d231..e2d6110d3a3d 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1397,7 +1397,8 @@ void __weak free_initmem(void)
 
 static int __ref kernel_init(void *unused)
 {
-	int ret;
+	char str[5];
+	int ret, len;
 
 	kernel_init_freeable();
 	/* need to finish all async __init code before freeing the memory */
@@ -1419,6 +1420,11 @@ static int __ref kernel_init(void *unused)
 
 	do_sysctl_args();
 
+	len = snprintf(str, 5, "A longer string\n");
+
+	pr_info("%s: len: %d, str: [%s]\n",
+		__func__, len, str);
+
 	if (ramdisk_execute_command) {
 		ret = run_init_process(ramdisk_execute_command);
 		if (!ret)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
  2020-09-17 16:25     ` Borislav Petkov
@ 2020-09-18  2:37       ` Xiongfeng Wang
  2020-09-18  7:12         ` Borislav Petkov
  0 siblings, 1 reply; 8+ messages in thread
From: Xiongfeng Wang @ 2020-09-18  2:37 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: mchehab, tony.luck, linux-edac, linux-kernel



On 2020/9/18 0:25, Borislav Petkov wrote:
> On Thu, Sep 17, 2020 at 07:38:57PM +0800, Xiongfeng Wang wrote:
>> I am not sure if snprintf will return a value larger than its second input
>> paramter 'size'.
> 
> The comment over snprintf() says
> 
>  * The return value is the number of characters which would be
>  * generated for the given input, excluding the trailing null,
>  * as per ISO C99.  If the return is greater than or equal to
>  * @size, the resulting string is truncated.
> 
> And let's try it, see diff at the end. Now look what that produces:
> 
> [    2.594796] kernel_init: len: 16, str: [A lo]
> 
> it returns 16 for len even though the buffer is 5 chars long. So in our
> patch, we'd increment by 16 which would be wrong.
> 
> Now let's use scnprintf():
> 
> [    2.700142] kernel_init: len: 4, str: [A lo]
> 
> Much better. Lemme do that.
> 
>> Not sure whether we need to check 'len' equals to 0.
>> if (len <= 0) ?
> 
> Yeah, lemme fix that too, so we have now incrementally ontop:


Thansk a lot. I will send another version. Also I will change the 'snprintf' in
'dimmdev_location_show()' to 'scnprintf'

Thanks,
Xiongfeng

> 
> ---
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index fa0551c81e63..c56e0004b39e 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -822,18 +822,17 @@ static ssize_t mci_max_location_show(struct device *dev,
>  	int i, n;
>  
>  	for (i = 0; i < mci->n_layers; i++) {
> -		n = snprintf(p, len, "%s %d ",
> -			     edac_layer_name[mci->layers[i].type],
> -			     mci->layers[i].size - 1);
> -
> +		n = scnprintf(p, len, "%s %d ",
> +			      edac_layer_name[mci->layers[i].type],
> +			      mci->layers[i].size - 1);
>  		len -= n;
> -		if (len < 0)
> +		if (len <= 0)
>  			goto out;
>  
>  		p += n;
>  	}
>  
> -	p += snprintf(p, len, "\n");
> +	p += scnprintf(p, len, "\n");
>  out:
>  	return p - data;
>  }
> ---
> 
> Test diff:
> 
> ---
> diff --git a/init/main.c b/init/main.c
> index ae78fb68d231..e2d6110d3a3d 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1397,7 +1397,8 @@ void __weak free_initmem(void)
>  
>  static int __ref kernel_init(void *unused)
>  {
> -	int ret;
> +	char str[5];
> +	int ret, len;
>  
>  	kernel_init_freeable();
>  	/* need to finish all async __init code before freeing the memory */
> @@ -1419,6 +1420,11 @@ static int __ref kernel_init(void *unused)
>  
>  	do_sysctl_args();
>  
> +	len = snprintf(str, 5, "A longer string\n");
> +
> +	pr_info("%s: len: %d, str: [%s]\n",
> +		__func__, len, str);
> +
>  	if (ramdisk_execute_command) {
>  		ret = run_init_process(ramdisk_execute_command);
>  		if (!ret)
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
  2020-09-18  2:37       ` Xiongfeng Wang
@ 2020-09-18  7:12         ` Borislav Petkov
  2020-09-18 13:31           ` Joe Perches
  0 siblings, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2020-09-18  7:12 UTC (permalink / raw)
  To: Xiongfeng Wang; +Cc: mchehab, tony.luck, linux-edac, linux-kernel

On Fri, Sep 18, 2020 at 10:37:28AM +0800, Xiongfeng Wang wrote:
> Thansk a lot. I will send another version. Also I will change the
> 'snprintf' in 'dimmdev_location_show()' to 'scnprintf'

No need to send another one - I have everything locally and just amended
it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location
  2020-09-18  7:12         ` Borislav Petkov
@ 2020-09-18 13:31           ` Joe Perches
  0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2020-09-18 13:31 UTC (permalink / raw)
  To: Borislav Petkov, Xiongfeng Wang
  Cc: mchehab, tony.luck, linux-edac, linux-kernel

On Fri, 2020-09-18 at 09:12 +0200, Borislav Petkov wrote:
> On Fri, Sep 18, 2020 at 10:37:28AM +0800, Xiongfeng Wang wrote:
> > Thansk a lot. I will send another version. Also I will change the
> > 'snprintf' in 'dimmdev_location_show()' to 'scnprintf'
> 
> No need to send another one - I have everything locally and just amended
> it.

A generic question about sysfs is whether or not the
PAGE_SIZE buf output should be newline terminated or
not if an the buffer is completely filled and the
desired output cannot be newline terminated.

Likely not.

NUL termination without newline should be enough to
indicate overrun.



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14  2:48 [PATCH v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location Xiongfeng Wang
2020-09-16 17:00 ` Borislav Petkov
2020-09-17 11:38   ` Xiongfeng Wang
2020-09-17 15:28     ` Joe Perches
2020-09-17 16:25     ` Borislav Petkov
2020-09-18  2:37       ` Xiongfeng Wang
2020-09-18  7:12         ` Borislav Petkov
2020-09-18 13:31           ` Joe Perches

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git