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

[David Sterba]

On Tue, Jan 02, 2024 at 02:42:10PM +1030, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Make _parse_integer_fixup_radix() to always treat "0x" as hex
>   This is to make sure invalid strings like "0x" or "0xG" to fail
>   as expected for memparse_safe().
>   Or they would only parse the first 0, then leaving "x" for caller
>   to handle.
> 
> - Update the test case to include above failure cases
>   This including:
>   * "0x", just hex prefix without any suffix/follow up chars
>   * "0xK", just hex prefix and a stray suffix
>   * "0xY", hex prefix with an invalid char
> 
> - Fix a bug in btrfs' conversion to memparse_safe()
>   Where I forgot to delete the old memparse() line.
> 
> - Fix a compiler warning on m68K
>   On that platform, a pointer (32 bits) is smaller than unsigned long long
>   (64 bits), which can cause static checker to warn.
> 
> 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.

As long as the suffixed values work in sysfs I don't care how it is
implemented. The safe version is nice of course, but it applies to the
'E' suffix which is so uncommon that there's no practical effect.