Re: [PATCH 0/3] kstrtox: introduce memparse_safe()

Re: [PATCH 0/3] kstrtox: introduce memparse_safe()

[Alexey Dobriyan]

> fs/btrfs/ioctl.c        |   8 +-
> fs/btrfs/super.c        |   8 ++
> fs/btrfs/sysfs.c        |  14 ++-
> include/linux/kernel.h  |   8 +-
> include/linux/kstrtox.h |  15 +++
> lib/cmdline.c           |   5 +-
> lib/kstrtox.c           |  96 ++++++++++++++++++
> lib/test-kstrtox.c      | 217 ++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 364 insertions(+), 7 deletions(-)

Please keep memparse stuff out of kstrtox.c .
kstrtox.c is for kstrto*() functions.

[PATCH 0/3] kstrtox: introduce memparse_safe()

[Qu Wenruo]

Function memparse() lacks error handling:

- If no valid number string at all
  In that case @retptr would just be updated and return value would be
  zero.

- No overflown detection
  This applies to both the number string part, and the suffixes part.
  And since we have no way to indicate errors, we can get weird results
  like:

  	"25E" -> 10376293541461622784 (9E)

  This is due to the fact that for "E" suffix, there is only 4 bits
  left, and 25 with 60 bits left shift would lead to overflow.
  (And decision to support for that "E" suffix is already cursed)

So here we introduce a safer version of it: memparse_safe(), and mark
the original one deprecated.
Unfortunately I didn't find a good way to mark it deprecated, as with
recent -Werror changes, '__deprecated' marco does not seem to warn
anymore.

The new helper has the following advantages:

- Better overflow and invalid string detection
  The overflow detection is for both the numberic part, and with the
  suffix. Thus above "25E" would be rejected correctly.
  The invalid string part means if there is no valid number starts at
  the buffer, we return -EINVAL.

- Allow caller to select the suffixes, and saner default ones
  The new default one would be "KMGTP", without the cursed and overflow
  prone "E".
  Some older code like setup_elfcorehdr() would benefit from it, if the
  code really wants to only allow "KMG" suffixes.

- Keep the old @retptr behavior
  So the existing callers can easily migrate to the new one, without the
  need to do extra strsep() work.

- Finally test cases
  The test case would cover more things other than the existing kstrtox
  tests:
  * The @retptr behavior
    Either for bad cases, which @retptr should not be touched,
    or for good cases, the @retptr is properly advanced,

  * Return value verification
    Make sure we distinguish -EINVAL and -ERANGE correctly.

With the new helper, migrate btrfs to the interface, and since the
@retptr behavior is the same, we won't cause any behavior change.

Qu Wenruo (3):
  kstrtox: introduce a safer version of memparse()
  kstrtox: add unit tests for memparse_safe()
  btrfs: migrate to the newer memparse_safe() helper

 fs/btrfs/ioctl.c        |   8 +-
 fs/btrfs/super.c        |   8 ++
 fs/btrfs/sysfs.c        |  14 ++-
 include/linux/kernel.h  |   8 +-
 include/linux/kstrtox.h |  15 +++
 lib/cmdline.c           |   5 +-
 lib/kstrtox.c           |  96 ++++++++++++++++++
 lib/test-kstrtox.c      | 217 ++++++++++++++++++++++++++++++++++++++++
 8 files changed, 364 insertions(+), 7 deletions(-)

-- 
2.43.0

Re: [PATCH 0/3] kstrtox: introduce memparse_safe()

[Andy Shevchenko]

On Sat, Dec 23, 2023 at 08:28:04PM +1030, Qu Wenruo wrote:
> Function memparse() lacks error handling:
> 
> - If no valid number string at all
>   In that case @retptr would just be updated and return value would be
>   zero.
> 
> - No overflown detection
>   This applies to both the number string part, and the suffixes part.
>   And since we have no way to indicate errors, we can get weird results
>   like:
> 
>   	"25E" -> 10376293541461622784 (9E)
> 
>   This is due to the fact that for "E" suffix, there is only 4 bits
>   left, and 25 with 60 bits left shift would lead to overflow.
>   (And decision to support for that "E" suffix is already cursed)
> 
> So here we introduce a safer version of it: memparse_safe(), and mark
> the original one deprecated.
> Unfortunately I didn't find a good way to mark it deprecated, as with
> recent -Werror changes, '__deprecated' marco does not seem to warn
> anymore.
> 
> The new helper has the following advantages:
> 
> - Better overflow and invalid string detection
>   The overflow detection is for both the numberic part, and with the
>   suffix. Thus above "25E" would be rejected correctly.
>   The invalid string part means if there is no valid number starts at
>   the buffer, we return -EINVAL.
> 
> - Allow caller to select the suffixes, and saner default ones
>   The new default one would be "KMGTP", without the cursed and overflow
>   prone "E".
>   Some older code like setup_elfcorehdr() would benefit from it, if the
>   code really wants to only allow "KMG" suffixes.
> 
> - Keep the old @retptr behavior
>   So the existing callers can easily migrate to the new one, without the
>   need to do extra strsep() work.
> 
> - Finally test cases
>   The test case would cover more things other than the existing kstrtox
>   tests:
>   * The @retptr behavior
>     Either for bad cases, which @retptr should not be touched,
>     or for good cases, the @retptr is properly advanced,
> 
>   * Return value verification
>     Make sure we distinguish -EINVAL and -ERANGE correctly.
> 
> With the new helper, migrate btrfs to the interface, and since the
> @retptr behavior is the same, we won't cause any behavior change.

Thank you for the prompt update, I will be off till end of January,
and in any case this is material for v6.9+, so I will look at this
afterwards.

-- 
With Best Regards,
Andy Shevchenko