[PATCH] libfs: fix negative value support in simple_attr_write()

[PATCH] libfs: fix negative value support in simple_attr_write()

[Eliav Farber]

After commit 488dac0c9237 ("libfs: fix error cast of negative value in
simple_attr_write()"), a user trying set a negative value will get a
'-EINVAL' error, because simple_attr_write() was modified to use
kstrtoull() which can handle only unsigned values, instead of
simple_strtoll().

This breaks all the places using DEFINE_DEBUGFS_ATTRIBUTE() with format
of a signed integer.

The u64 value which attr->set() receives is not an issue for negative
numbers.
The %lld and %llu in any case are for 64-bit value. Representing it as
unsigned simplifies the generic code, but it doesn't mean we can't keep
their signed value if we know that.

This change basically reverts the mentioned commit, but uses kstrtoll()
instead of simple_strtoll() which is obsolete.

Fixes: 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()")
Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 fs/libfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 31b0ddf01c31..3bccd75815db 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1016,7 +1016,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);
+	ret = kstrtoll(attr->set_buf, 0, &val);
 	if (ret)
 		goto out;
 	ret = attr->set(attr->data, val);
-- 
2.37.1

Re: [PATCH] libfs: fix negative value support in simple_attr_write()

[Yicong Yang]

On 2022/9/18 21:50, Eliav Farber wrote:
> After commit 488dac0c9237 ("libfs: fix error cast of negative value in
> simple_attr_write()"), a user trying set a negative value will get a
> '-EINVAL' error, because simple_attr_write() was modified to use
> kstrtoull() which can handle only unsigned values, instead of
> simple_strtoll().
> 
> This breaks all the places using DEFINE_DEBUGFS_ATTRIBUTE() with format
> of a signed integer.
> 
> The u64 value which attr->set() receives is not an issue for negative
> numbers.
> The %lld and %llu in any case are for 64-bit value. Representing it as
> unsigned simplifies the generic code, but it doesn't mean we can't keep
> their signed value if we know that.
> 
> This change basically reverts the mentioned commit, but uses kstrtoll()
> instead of simple_strtoll() which is obsolete.
> 
> Fixes: 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()")

Thanks for fixing this.

Reviewed-by: Yicong Yang <yangyicong@hisilicon.com>

Sorry for my lack of knowledge for the -1 usage.

Thanks.

> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  fs/libfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 31b0ddf01c31..3bccd75815db 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1016,7 +1016,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);
> +	ret = kstrtoll(attr->set_buf, 0, &val);
>  	if (ret)
>  		goto out;
>  	ret = attr->set(attr->data, val);
> 

Re: [PATCH] libfs: fix negative value support in simple_attr_write()

[Andrew Morton]

On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <farbere@amazon.com> wrote:

> After commit 488dac0c9237 ("libfs: fix error cast of negative value in
> simple_attr_write()"), a user trying set a negative value will get a
> '-EINVAL' error, because simple_attr_write() was modified to use
> kstrtoull() which can handle only unsigned values, instead of
> simple_strtoll().
> 
> This breaks all the places using DEFINE_DEBUGFS_ATTRIBUTE() with format
> of a signed integer.
> 
> The u64 value which attr->set() receives is not an issue for negative
> numbers.
> The %lld and %llu in any case are for 64-bit value. Representing it as
> unsigned simplifies the generic code, but it doesn't mean we can't keep
> their signed value if we know that.
> 
> This change basically reverts the mentioned commit, but uses kstrtoll()
> instead of simple_strtoll() which is obsolete.
> 

https://lkml.kernel.org/r/20220919172418.45257-1-akinobu.mita@gmail.com
addresses the same thing.

Should the final version of this fix be backported into -stable trees?

Re: [PATCH] libfs: fix negative value support in simple_attr_write()

[Farber, Eliav]

On 9/20/2022 12:24 AM, Andrew Morton wrote:
> On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <farbere@amazon.com> 
> wrote:
>
>> After commit 488dac0c9237 ("libfs: fix error cast of negative value in
>> simple_attr_write()"), a user trying set a negative value will get a
>> '-EINVAL' error, because simple_attr_write() was modified to use
>> kstrtoull() which can handle only unsigned values, instead of
>> simple_strtoll().
>>
>> This breaks all the places using DEFINE_DEBUGFS_ATTRIBUTE() with format
>> of a signed integer.
>>
>> The u64 value which attr->set() receives is not an issue for negative
>> numbers.
>> The %lld and %llu in any case are for 64-bit value. Representing it as
>> unsigned simplifies the generic code, but it doesn't mean we can't keep
>> their signed value if we know that.
>>
>> This change basically reverts the mentioned commit, but uses kstrtoll()
>> instead of simple_strtoll() which is obsolete.
>>
>
> https://lkml.kernel.org/r/20220919172418.45257-1-akinobu.mita@gmail.com
> addresses the same thing.
>
> Should the final version of this fix be backported into -stable trees?


I think that my patch can be dropped in favor of Akinobu's patch.

--
Regards, Eliav

VS: [PATCH] libfs: fix negative value support in simple_attr_write()

[Shevchenko, Andriy]

________________________________________
Lähettäjä: Andrew Morton <akpm@linux-foundation.org>
Lähetetty: tiistai 20. syyskuuta 2022 0.24
Vastaanottaja: Eliav Farber
Kopio: viro@zeniv.linux.org.uk; yangyicong@hisilicon.com; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; Shevchenko, Andriy; hhhawa@amazon.com; jonnyc@amazon.com; Akinobu Mita
Aihe: Re: [PATCH] libfs: fix negative value support in simple_attr_write()

On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <farbere@amazon.com> wrote:

> After commit 488dac0c9237 ("libfs: fix error cast of negative value in
> simple_attr_write()"), a user trying set a negative value will get a
> '-EINVAL' error, because simple_attr_write() was modified to use
> kstrtoull() which can handle only unsigned values, instead of
> simple_strtoll().
>
> This breaks all the places using DEFINE_DEBUGFS_ATTRIBUTE() with format
> of a signed integer.
>
> The u64 value which attr->set() receives is not an issue for negative
> numbers.
> The %lld and %llu in any case are for 64-bit value. Representing it as
> unsigned simplifies the generic code, but it doesn't mean we can't keep
> their signed value if we know that.
>
> This change basically reverts the mentioned commit, but uses kstrtoll()
> instead of simple_strtoll() which is obsolete.
>

https://lkml.kernel.org/r/20220919172418.45257-1-akinobu.mita@gmail.com
addresses the same thing.

Should the final version of this fix be backported into -stable trees?

Oh, this rises the question, why the heck we even have the format parameter to those macros? Seems to me like a (hackish) workaround against the known issue which was introduced by the previously mentioned change.

Re: [PATCH] libfs: fix negative value support in simple_attr_write()

[Andy Shevchenko]

On Mon, Sep 19, 2022 at 02:24:13PM -0700, Andrew Morton wrote:
> On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <farbere@amazon.com> wrote:
> 
> > After commit 488dac0c9237 ("libfs: fix error cast of negative value in
> > simple_attr_write()"), a user trying set a negative value will get a
> > '-EINVAL' error, because simple_attr_write() was modified to use
> > kstrtoull() which can handle only unsigned values, instead of
> > simple_strtoll().
> > 
> > This breaks all the places using DEFINE_DEBUGFS_ATTRIBUTE() with format
> > of a signed integer.
> > 
> > The u64 value which attr->set() receives is not an issue for negative
> > numbers.
> > The %lld and %llu in any case are for 64-bit value. Representing it as
> > unsigned simplifies the generic code, but it doesn't mean we can't keep
> > their signed value if we know that.
> > 
> > This change basically reverts the mentioned commit, but uses kstrtoll()
> > instead of simple_strtoll() which is obsolete.
> > 
> 
> https://lkml.kernel.org/r/20220919172418.45257-1-akinobu.mita@gmail.com
> addresses the same thing.
> 
> Should the final version of this fix be backported into -stable trees?

But it questioning the formatting string as a parameter. Why do we need that
in the first place then?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] libfs: fix negative value support in simple_attr_write()

[Andy Shevchenko]

On Wed, Sep 21, 2022 at 05:39:09PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 19, 2022 at 02:24:13PM -0700, Andrew Morton wrote:
> > On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <farbere@amazon.com> wrote:

...

> > https://lkml.kernel.org/r/20220919172418.45257-1-akinobu.mita@gmail.com

> > Should the final version of this fix be backported into -stable trees?
> 
> But it questioning the formatting string as a parameter. Why do we need that
> in the first place then?

Sorry if above is the similar to something I have sent earlier, I had had an
issue with my IMAP and SMTP servers access.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] libfs: fix negative value support in simple_attr_write()

[Andy Shevchenko]

On Sun, Sep 18, 2022 at 01:50:36PM +0000, Eliav Farber wrote:
> After commit 488dac0c9237 ("libfs: fix error cast of negative value in
> simple_attr_write()"), a user trying set a negative value will get a
> '-EINVAL' error, because simple_attr_write() was modified to use
> kstrtoull() which can handle only unsigned values, instead of
> simple_strtoll().
> 
> This breaks all the places using DEFINE_DEBUGFS_ATTRIBUTE() with format
> of a signed integer.
> 
> The u64 value which attr->set() receives is not an issue for negative
> numbers.
> The %lld and %llu in any case are for 64-bit value. Representing it as
> unsigned simplifies the generic code, but it doesn't mean we can't keep
> their signed value if we know that.
> 
> This change basically reverts the mentioned commit, but uses kstrtoll()
> instead of simple_strtoll() which is obsolete.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

and I prefer this one over spreading more macros with redundant formatting
parameter.

> Fixes: 488dac0c9237 ("libfs: fix error cast of negative value in simple_attr_write()")
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  fs/libfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 31b0ddf01c31..3bccd75815db 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1016,7 +1016,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);
> +	ret = kstrtoll(attr->set_buf, 0, &val);
>  	if (ret)
>  		goto out;
>  	ret = attr->set(attr->data, val);
> -- 
> 2.37.1
> 

-- 
With Best Regards,
Andy Shevchenko