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

Andy Shevchenko
2023-12-27T16:12:09Z

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