Re: [PATCH] Revert "libfs: fix error cast of negative value in simple_attr_write()"

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mikhail,

On 2021/3/17 4:49, m.malygin@xxxxxxxxx wrote:
> From: Mikhail Malygin <m.malygin@xxxxxxxxx>
> 
> This reverts commit 488dac0c9237647e9b8f788b6a342595bfa40bda.
> 
> An established and documented [1] way of of configuring unlimited number of failures in fault-injection framework is to write -1:
> 
> - /sys/kernel/debug/fail*/times:
> 
>  specifies how many times failures may happen at most.
>  A value of -1 means "no limit".
> 
> Commit 488dac0c92 inadverently breaks that.
> 
> 1. https://www.kernel.org/doc/Documentation/fault-injection/fault-injection.txt

a simple revert can address this issue, but i don't think it's reasonable to directly cast the negative value.

considering attr->set() callback receives a value of u64, it's hard for the upper users
to know whether it's casted or not.

> 
> Signed-off-by: Mikhail Malygin <m.malygin@xxxxxxxxx>
> Signef-off-by: Roman Bolshakov <r.bolshakov@xxxxxxxxx>
> ---
>  fs/libfs.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index e2de5401abca..9bea71111299 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -961,7 +961,7 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf,
>  			  size_t len, loff_t *ppos)
>  {
>  	struct simple_attr *attr;
> -	unsigned long long val;
> +	u64 val;
>  	size_t size;
>  	ssize_t ret;
>  
> @@ -979,9 +979,7 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf,
>  		goto out;
>  
>  	attr->set_buf[size] = '\0';
> -	ret = kstrtoull(attr->set_buf, 0, &val);
> -	if (ret)
> -		goto out;
> +	val = simple_strtoll(attr->set_buf, NULL, 0);

simple_strtoll() is deprecated and has unexpected results[1],
use kstrtoll() instead.

[1] https://github.com/torvalds/linux/blob/master/Documentation/process/deprecated.rst#simple_strtol-simple_strtoll-simple_strtoul-simple_strtoull

Thanks,
Yicong

>  	ret = attr->set(attr->data, val);
>  	if (ret == 0)
>  		ret = len; /* on success, claim we got the whole input */
> 




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux