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 <[email protected]>
---
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
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 <[email protected]>
Sorry for my lack of knowledge for the -1 usage.
Thanks.
> Signed-off-by: Eliav Farber <[email protected]>
> ---
> 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);
>
On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <[email protected]> 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/[email protected]
addresses the same thing.
Should the final version of this fix be backported into -stable trees?
On 9/20/2022 12:24 AM, Andrew Morton wrote:
> On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <[email protected]>
> 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/[email protected]
> 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
________________________________________
L?hett?j?: Andrew Morton <[email protected]>
L?hetetty: tiistai 20. syyskuuta 2022 0.24
Vastaanottaja: Eliav Farber
Kopio: [email protected]; [email protected]; [email protected]; [email protected]; Shevchenko, Andriy; [email protected]; [email protected]; 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 <[email protected]> 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/[email protected]
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.
On Mon, Sep 19, 2022 at 02:24:13PM -0700, Andrew Morton wrote:
> On Sun, 18 Sep 2022 13:50:36 +0000 Eliav Farber <[email protected]> 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/[email protected]
> 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
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 <[email protected]> wrote:
...
> > https://lkml.kernel.org/r/[email protected]
> > 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
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 <[email protected]>
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 <[email protected]>
> ---
> 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