| Message ID | 1600051734-8993-1-git-send-email-wangxiongfeng2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] EDAC/mc_sysfs: Add missing newlines when printing {max,dimm}_location | expand |
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;
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
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.
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)
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) >
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.
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.
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; }
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(-)