| Message ID | 20220826170716.6886-3-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/4] pwm: sysfs: Switch to DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() | expand |
On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote: > For sysfs outputs, it's safer to use a new helper, sysfs_emit(), > instead of the raw sprintf() & co. This patch replaces such a > sprintf() call straightforwardly with the new helper. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > v2: added tag (Uwe) > drivers/pwm/sysfs.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) How exactly is sysfs_emit() safer here? In all of these cases, the values that sprintf() writes are the only values that are written into the buffer and we know that none of them exceed PAGE_SIZE. So the additional checks that sysfs_emit() performs are useless. Thierry
On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote: > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote: > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(), > > instead of the raw sprintf() & co. This patch replaces such a > > sprintf() call straightforwardly with the new helper. > How exactly is sysfs_emit() safer here? In all of these cases, the > values that sprintf() writes are the only values that are written into > the buffer and we know that none of them exceed PAGE_SIZE. So the > additional checks that sysfs_emit() performs are useless. This is a recommended way to use sysfs_emit() mentioned in Documentation. Care to fix documentation?
On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote: > On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote: > > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote: > > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(), > > > instead of the raw sprintf() & co. This patch replaces such a > > > sprintf() call straightforwardly with the new helper. > > > How exactly is sysfs_emit() safer here? In all of these cases, the > > values that sprintf() writes are the only values that are written into > > the buffer and we know that none of them exceed PAGE_SIZE. So the > > additional checks that sysfs_emit() performs are useless. > > This is a recommended way to use sysfs_emit() mentioned in Documentation. > Care to fix documentation? For your convenience, Documentation/filesystems/sysfs.rst says: - show() should only use sysfs_emit() or sysfs_emit_at() when formatting the value to be returned to user space.
On Wed, Sep 28, 2022 at 04:58:17PM +0300, Andy Shevchenko wrote: > On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote: > > > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote: > > > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(), > > > > instead of the raw sprintf() & co. This patch replaces such a > > > > sprintf() call straightforwardly with the new helper. > > > > > How exactly is sysfs_emit() safer here? In all of these cases, the > > > values that sprintf() writes are the only values that are written into > > > the buffer and we know that none of them exceed PAGE_SIZE. So the > > > additional checks that sysfs_emit() performs are useless. > > > > This is a recommended way to use sysfs_emit() mentioned in Documentation. > > Care to fix documentation? > > For your convenience, Documentation/filesystems/sysfs.rst says: > > - show() should only use sysfs_emit() or sysfs_emit_at() when formatting > the value to be returned to user space. Took some digging to find enough information to convince me. Again, the commit message says that sysfs_emit() is safer, but that's a bad reason in this case because these cases are fine. The sprintf() calls that this replaces aren't unbound and we're not appending to an existing seq_buf, so nothing to worry on that front. I think the better argument for broadly applying this is to specifically distinguish the sysfs sprintf() calls from others so that they can be auditioned better and perhaps help with the documentation[0]. Do you mind if I apply this with a reworded documentation? Thierry [0]: https://lore.kernel.org/all/20200930115740.GA1611809@kroah.com/
On Wed, Sep 28, 2022 at 05:20:07PM +0200, Thierry Reding wrote: > On Wed, Sep 28, 2022 at 04:58:17PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote: > > > On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote: > > > > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote: > > > > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(), > > > > > instead of the raw sprintf() & co. This patch replaces such a > > > > > sprintf() call straightforwardly with the new helper. > > > > > > > How exactly is sysfs_emit() safer here? In all of these cases, the > > > > values that sprintf() writes are the only values that are written into > > > > the buffer and we know that none of them exceed PAGE_SIZE. So the > > > > additional checks that sysfs_emit() performs are useless. > > > > > > This is a recommended way to use sysfs_emit() mentioned in Documentation. > > > Care to fix documentation? > > > > For your convenience, Documentation/filesystems/sysfs.rst says: > > > > - show() should only use sysfs_emit() or sysfs_emit_at() when formatting > > the value to be returned to user space. > > Took some digging to find enough information to convince me. Again, the > commit message says that sysfs_emit() is safer, but that's a bad reason > in this case because these cases are fine. The sprintf() calls that this > replaces aren't unbound and we're not appending to an existing seq_buf, > so nothing to worry on that front. > > I think the better argument for broadly applying this is to specifically > distinguish the sysfs sprintf() calls from others so that they can be > auditioned better and perhaps help with the documentation[0]. > > Do you mind if I apply this with a reworded documentation? I meant "commit message", not documentation. Thierry
On Wed, Sep 28, 2022 at 05:20:07PM +0200, Thierry Reding wrote: > On Wed, Sep 28, 2022 at 04:58:17PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 28, 2022 at 04:40:35PM +0300, Andy Shevchenko wrote: > > > On Wed, Sep 28, 2022 at 02:28:41PM +0200, Thierry Reding wrote: > > > > On Fri, Aug 26, 2022 at 08:07:15PM +0300, Andy Shevchenko wrote: > > > > > For sysfs outputs, it's safer to use a new helper, sysfs_emit(), > > > > > instead of the raw sprintf() & co. This patch replaces such a > > > > > sprintf() call straightforwardly with the new helper. > > > > > > > How exactly is sysfs_emit() safer here? In all of these cases, the > > > > values that sprintf() writes are the only values that are written into > > > > the buffer and we know that none of them exceed PAGE_SIZE. So the > > > > additional checks that sysfs_emit() performs are useless. > > > > > > This is a recommended way to use sysfs_emit() mentioned in Documentation. > > > Care to fix documentation? > > > > For your convenience, Documentation/filesystems/sysfs.rst says: > > > > - show() should only use sysfs_emit() or sysfs_emit_at() when formatting > > the value to be returned to user space. > > Took some digging to find enough information to convince me. Again, the > commit message says that sysfs_emit() is safer, but that's a bad reason > in this case because these cases are fine. The sprintf() calls that this > replaces aren't unbound and we're not appending to an existing seq_buf, > so nothing to worry on that front. > > I think the better argument for broadly applying this is to specifically > distinguish the sysfs sprintf() calls from others so that they can be > auditioned better and perhaps help with the documentation[0]. > > Do you mind if I apply this with a reworded documentation? I do not mind, go ahead with it. Thank you! > Thierry > > [0]: https://lore.kernel.org/all/20200930115740.GA1611809@kroah.com/
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 1543fe07765b..767c4b19afb1 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -42,7 +42,7 @@ static ssize_t period_show(struct device *child, pwm_get_state(pwm, &state); - return sprintf(buf, "%llu\n", state.period); + return sysfs_emit(buf, "%llu\n", state.period); } static ssize_t period_store(struct device *child, @@ -77,7 +77,7 @@ static ssize_t duty_cycle_show(struct device *child, pwm_get_state(pwm, &state); - return sprintf(buf, "%llu\n", state.duty_cycle); + return sysfs_emit(buf, "%llu\n", state.duty_cycle); } static ssize_t duty_cycle_store(struct device *child, @@ -112,7 +112,7 @@ static ssize_t enable_show(struct device *child, pwm_get_state(pwm, &state); - return sprintf(buf, "%d\n", state.enabled); + return sysfs_emit(buf, "%d\n", state.enabled); } static ssize_t enable_store(struct device *child, @@ -171,7 +171,7 @@ static ssize_t polarity_show(struct device *child, break; } - return sprintf(buf, "%s\n", polarity); + return sysfs_emit(buf, "%s\n", polarity); } static ssize_t polarity_store(struct device *child, @@ -212,7 +212,7 @@ static ssize_t capture_show(struct device *child, if (ret) return ret; - return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); + return sysfs_emit(buf, "%u %u\n", result.period, result.duty_cycle); } static DEVICE_ATTR_RW(period); @@ -361,7 +361,7 @@ static ssize_t npwm_show(struct device *parent, struct device_attribute *attr, { const struct pwm_chip *chip = dev_get_drvdata(parent); - return sprintf(buf, "%u\n", chip->npwm); + return sysfs_emit(buf, "%u\n", chip->npwm); } static DEVICE_ATTR_RO(npwm);