linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] btrfs: replace snprintf in show functions with sysfs_emit
@ 2021-10-13  3:28 Qing Wang
  2021-10-13  7:51 ` Anand Jain
  0 siblings, 1 reply; 6+ messages in thread
From: Qing Wang @ 2021-10-13  3:28 UTC (permalink / raw)
  To: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel
  Cc: Qing Wang

coccicheck complains about the use of snprintf() in sysfs show functions.

Fix the following coccicheck warning:
fs/btrfs/sysfs.c:335:8-16: WARNING: use scnprintf or sprintf.

Use sysfs_emit instead of scnprintf or sprintf makes more sense.

Signed-off-by: Qing Wang <wangqing@vivo.com>
---
 fs/btrfs/sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 9d1d140..fda094a 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -332,7 +332,7 @@ BTRFS_ATTR(static_feature, supported_checksums, supported_checksums_show);
 static ssize_t send_stream_version_show(struct kobject *kobj,
 					struct kobj_attribute *ka, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n", BTRFS_SEND_STREAM_VERSION);
+	return sysfs_emit(buf, "%d\n", BTRFS_SEND_STREAM_VERSION);
 }
 BTRFS_ATTR(static_feature, send_stream_version, send_stream_version_show);
 
-- 
2.7.4


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

* Re: [PATCH] btrfs: replace snprintf in show functions with sysfs_emit
  2021-10-13  3:28 [PATCH] btrfs: replace snprintf in show functions with sysfs_emit Qing Wang
@ 2021-10-13  7:51 ` Anand Jain
  2021-10-13 10:36   ` David Sterba
  0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2021-10-13  7:51 UTC (permalink / raw)
  To: Qing Wang, Chris Mason, Josef Bacik, David Sterba, linux-btrfs,
	linux-kernel

On 13/10/2021 11:28, Qing Wang wrote:
> coccicheck complains about the use of snprintf() in sysfs show functions.

It looks like the reason is snprintf() unaware of the PAGE_SIZE 
max_limit of the buf.

> Fix the following coccicheck warning:
> fs/btrfs/sysfs.c:335:8-16: WARNING: use scnprintf or sprintf.

Hm. We use snprintf() at quite a lot more places in sysfs.c and, I don't 
see them getting this fix. Why?

> Use sysfs_emit instead of scnprintf or sprintf makes more sense.

Below commit has added it. Nice.

commit 2efc459d06f1630001e3984854848a5647086232
Date:   Wed Sep 16 13:40:38 2020 -0700

     sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs out

Thanks, Anand

> 
> Signed-off-by: Qing Wang <wangqing@vivo.com>
> ---
>   fs/btrfs/sysfs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 9d1d140..fda094a 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -332,7 +332,7 @@ BTRFS_ATTR(static_feature, supported_checksums, supported_checksums_show);
>   static ssize_t send_stream_version_show(struct kobject *kobj,
>   					struct kobj_attribute *ka, char *buf)
>   {
> -	return snprintf(buf, PAGE_SIZE, "%d\n", BTRFS_SEND_STREAM_VERSION);
> +	return sysfs_emit(buf, "%d\n", BTRFS_SEND_STREAM_VERSION);
>   }
>   BTRFS_ATTR(static_feature, send_stream_version, send_stream_version_show);
>   
> 


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

* Re: [PATCH] btrfs: replace snprintf in show functions with sysfs_emit
  2021-10-13  7:51 ` Anand Jain
@ 2021-10-13 10:36   ` David Sterba
  2021-10-13 10:49     ` Qu Wenruo
       [not found]     ` <ADsAzABEEmLRWHzgUOl4Sqr5.9.1634122164687.Hmail.wangqing@vivo.com>
  0 siblings, 2 replies; 6+ messages in thread
From: David Sterba @ 2021-10-13 10:36 UTC (permalink / raw)
  To: Anand Jain
  Cc: Qing Wang, Chris Mason, Josef Bacik, David Sterba, linux-btrfs,
	linux-kernel

On Wed, Oct 13, 2021 at 03:51:33PM +0800, Anand Jain wrote:
> On 13/10/2021 11:28, Qing Wang wrote:
> > coccicheck complains about the use of snprintf() in sysfs show functions.
> 
> It looks like the reason is snprintf() unaware of the PAGE_SIZE 
> max_limit of the buf.
> 
> > Fix the following coccicheck warning:
> > fs/btrfs/sysfs.c:335:8-16: WARNING: use scnprintf or sprintf.
> 
> Hm. We use snprintf() at quite a lot more places in sysfs.c and, I don't 
> see them getting this fix. Why?

I guess the patch is only addressing the warning for snprintf, reading
the sources would show how many more conversions could have been done of
scnprintf calls.

> > Use sysfs_emit instead of scnprintf or sprintf makes more sense.
> 
> Below commit has added it. Nice.
> 
> commit 2efc459d06f1630001e3984854848a5647086232
> Date:   Wed Sep 16 13:40:38 2020 -0700
> 
>      sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs out

The conversion to the standard helper is good, but should be done
in the entire file.

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

* Re: [PATCH] btrfs: replace snprintf in show functions with sysfs_emit
  2021-10-13 10:36   ` David Sterba
@ 2021-10-13 10:49     ` Qu Wenruo
       [not found]     ` <ADsAzABEEmLRWHzgUOl4Sqr5.9.1634122164687.Hmail.wangqing@vivo.com>
  1 sibling, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2021-10-13 10:49 UTC (permalink / raw)
  To: dsterba, Anand Jain, Qing Wang, Chris Mason, Josef Bacik,
	David Sterba, linux-btrfs, linux-kernel



On 2021/10/13 18:36, David Sterba wrote:
> On Wed, Oct 13, 2021 at 03:51:33PM +0800, Anand Jain wrote:
>> On 13/10/2021 11:28, Qing Wang wrote:
>>> coccicheck complains about the use of snprintf() in sysfs show functions.
>>
>> It looks like the reason is snprintf() unaware of the PAGE_SIZE
>> max_limit of the buf.
>>
>>> Fix the following coccicheck warning:
>>> fs/btrfs/sysfs.c:335:8-16: WARNING: use scnprintf or sprintf.

IIRC sprintf() is less safe than snprintf().
Is the check really correct to mention sprintf()?

>>
>> Hm. We use snprintf() at quite a lot more places in sysfs.c and, I don't
>> see them getting this fix. Why?
>
> I guess the patch is only addressing the warning for snprintf, reading
> the sources would show how many more conversions could have been done of
> scnprintf calls.
>
>>> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
>>
>> Below commit has added it. Nice.
>>
>> commit 2efc459d06f1630001e3984854848a5647086232
>> Date:   Wed Sep 16 13:40:38 2020 -0700
>>
>>       sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs out
>
> The conversion to the standard helper is good, but should be done
> in the entire file.
>

Yeah, the same idea, all sysfs interface should convert to the new
interface, not only the snprintf().

Thanks,
Qu

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

* 回复: [PATCH] btrfs: replace snprintf in show functions with sysfs_emit
       [not found]     ` <ADsAzABEEmLRWHzgUOl4Sqr5.9.1634122164687.Hmail.wangqing@vivo.com>
@ 2021-10-13 11:01       ` 王擎
  2021-10-13 11:08         ` Qu Wenruo
  0 siblings, 1 reply; 6+ messages in thread
From: 王擎 @ 2021-10-13 11:01 UTC (permalink / raw)
  To: Qu Wenruo, dsterba, Anand Jain, Chris Mason, Josef Bacik,
	David Sterba, linux-btrfs, linux-kernel


>> On Wed, Oct 13, 2021 at 03:51:33PM +0800, Anand Jain wrote:
>>> On 13/10/2021 11:28, Qing Wang wrote:
>>>> coccicheck complains about the use of snprintf() in sysfs show functions.
>>>
>>> It looks like the reason is snprintf() unaware of the PAGE_SIZE
>>> max_limit of the buf.
>>>
>>>> Fix the following coccicheck warning:
>>>> fs/btrfs/sysfs.c:335:8-16: WARNING: use scnprintf or sprintf.
>
>IIRC sprintf() is less safe than snprintf().
>Is the check really correct to mention sprintf()?

device_attr_show.cocci metions show() must not use snprintf() 
when formatting the value to be returned to user space.
If you can guarantee that an overflow will never happen you
can use sprintf() otherwise you must use scnprintf().

My understanding is this is not only to solve the possible 
overflow issue, snprintf() returns the length of the string, not 
the length actually written. We can directly use sysfs_emit() here.

Thanks,

Qing

>>>
>>> Hm. We use snprintf() at quite a lot more places in sysfs.c and, I don't
>>> see them getting this fix. Why?
>>
>> I guess the patch is only addressing the warning for snprintf, reading
>> the sources would show how many more conversions could have been done of
>> scnprintf calls.
>>
>>>> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
>>>
>>> Below commit has added it. Nice.
>>>
>>> commit 2efc459d06f1630001e3984854848a5647086232
>>> Date:   Wed Sep 16 13:40:38 2020 -0700
>>>
>>>       sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs out
>>
>> The conversion to the standard helper is good, but should be done
>> in the entire file.
>>
>
> Yeah, the same idea, all sysfs interface should convert to the new
> interface, not only the snprintf().
>
> Thanks,
> Qu

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

* Re: [PATCH] btrfs: replace snprintf in show functions with sysfs_emit
  2021-10-13 11:01       ` 回复: " 王擎
@ 2021-10-13 11:08         ` Qu Wenruo
  0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2021-10-13 11:08 UTC (permalink / raw)
  To: 王擎,
	dsterba, Anand Jain, Chris Mason, Josef Bacik, David Sterba,
	linux-btrfs, linux-kernel



On 2021/10/13 19:01, 王擎 wrote:
>
>>> On Wed, Oct 13, 2021 at 03:51:33PM +0800, Anand Jain wrote:
>>>> On 13/10/2021 11:28, Qing Wang wrote:
>>>>> coccicheck complains about the use of snprintf() in sysfs show functions.
>>>>
>>>> It looks like the reason is snprintf() unaware of the PAGE_SIZE
>>>> max_limit of the buf.
>>>>
>>>>> Fix the following coccicheck warning:
>>>>> fs/btrfs/sysfs.c:335:8-16: WARNING: use scnprintf or sprintf.
>>
>> IIRC sprintf() is less safe than snprintf().
>> Is the check really correct to mention sprintf()?
>
> device_attr_show.cocci metions show() must not use snprintf()
> when formatting the value to be returned to user space.
> If you can guarantee that an overflow will never happen you
> can use sprintf() otherwise you must use scnprintf().

I totally understand snprintf() has its problem for not returning the
real written size, thus not safe.

But sprintf() is worse, it doesn't even prevent overflow from the beginning.

In fact, for case that could overflow, snprintf() would only overflow if
we have extra bytes to output and doesn't check if the offset is beyond
PAGE_SIZE at snprintf() call.

But for sprintf(), it would cause overflow immediately.

Thus mentioning sprintf() is more problematic.
Only scnprintf() is safe.


But sure, sysfs_emit() and sysfs_emit_at() would be a better solution.

Thanks,
Qu

>
> My understanding is this is not only to solve the possible
> overflow issue, snprintf() returns the length of the string, not
> the length actually written. We can directly use sysfs_emit() here.
>
> Thanks,
>
> Qing
>
>>>>
>>>> Hm. We use snprintf() at quite a lot more places in sysfs.c and, I don't
>>>> see them getting this fix. Why?
>>>
>>> I guess the patch is only addressing the warning for snprintf, reading
>>> the sources would show how many more conversions could have been done of
>>> scnprintf calls.
>>>
>>>>> Use sysfs_emit instead of scnprintf or sprintf makes more sense.
>>>>
>>>> Below commit has added it. Nice.
>>>>
>>>> commit 2efc459d06f1630001e3984854848a5647086232
>>>> Date:   Wed Sep 16 13:40:38 2020 -0700
>>>>
>>>>         sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs out
>>>
>>> The conversion to the standard helper is good, but should be done
>>> in the entire file.
>>>
>>
>> Yeah, the same idea, all sysfs interface should convert to the new
>> interface, not only the snprintf().
>>
>> Thanks,
>> Qu

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

end of thread, other threads:[~2021-10-13 11:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13  3:28 [PATCH] btrfs: replace snprintf in show functions with sysfs_emit Qing Wang
2021-10-13  7:51 ` Anand Jain
2021-10-13 10:36   ` David Sterba
2021-10-13 10:49     ` Qu Wenruo
     [not found]     ` <ADsAzABEEmLRWHzgUOl4Sqr5.9.1634122164687.Hmail.wangqing@vivo.com>
2021-10-13 11:01       ` 回复: " 王擎
2021-10-13 11:08         ` Qu Wenruo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).