by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()

Hi Qu,

On Tue, Jan 2, 2024 at 9:56 PM Qu Wenruo <[email protected]> wrote:
> On 2024/1/2 23:53, Geert Uytterhoeven wrote:
> > On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <[email protected]> wrote:
> >> The new tests cases for memparse_safe() include:
> >>
> >> - The existing test cases for kstrtoull()
> >> Including all the 3 bases (8, 10, 16), and all the ok and failure
> >> cases.
> >> Although there are something we need to verify specific for
> >> memparse_safe():
> >>
> >> * @retptr and @value are not modified for failure cases
> >>
> >> * return value are correct for failure cases
> >>
> >> * @retptr is correct for the good cases
> >>
> >> - New test cases
> >> Not only testing the result value, but also the @retptr, including:
> >>
> >> * good cases with extra tailing chars, but without valid prefix
> >> The @retptr should point to the first char after a valid string.
> >> 3 cases for all the 3 bases.
> >>
> >> * good cases with extra tailing chars, with valid prefix
> >> 5 cases for all the suffixes.
> >>
> >> * bad cases without any number but stray suffix
> >> Should be rejected with -EINVAL
> >>
> >> Signed-off-by: Qu Wenruo <[email protected]>
> >
> > Thanks for your patch!
> >
> >> --- a/lib/test-kstrtox.c
> >> +++ b/lib/test-kstrtox.c
> >> @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void)
> >> TEST_OK(kstrtoll, long long, "%lld", test_ll_ok);
> >> }
> >>
> >> +/*
> >> + * The special pattern to make sure the result is not modified for error cases.
> >> + */
> >> +#define ULL_PATTERN (0xefefefef7a7a7a7aULL)
> >> +#if BITS_PER_LONG == 32
> >> +#define POINTER_PATTERN (0xefef7a7a7aUL)
> >
> > This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit
> > unsigned long or pointer. Probably you wanted to use 0xef7a7a7aUL
> > instead?
>
> My bad, one extra byte...

So did that fix the sparse warning? ;-)

> >> +#else
> >> +#define POINTER_PATTERN (ULL_PATTERN)
> >> +#endif
> >
> > Shouldn't a simple cast to uintptr_t work fine for both 32-bit and
> > 64-bit systems:
> >
> > #define POINTER_PATTERN ((uintptr_t)ULL_PATTERN)
> >
> > Or even better, incorporate the cast to a pointer:
> >
> > #define POINTER_PATTERN ((void *)(uintptr_t)ULL_PATTERN)
>
> The problem is reported by sparse, which warns about that ULL_PATTERN
> converted to a pointer would lose its width:
>
> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from
> constant value (efefefef7a7a7a7a becomes 7a7a7a7a)

Ah yes, sparse can be annoying.
I'm still looking for a clean and concise way to shut up [1].

> I'm not sure if using uiintptr_t would solve it, thus I go the macro to
> switch the value to avoid the static checker's warning.
>
> I tried to check how other locations handles patterned pointer value,
> like CONFIG_INIT_STACK_ALL_PATTERN, but they're either relying on the
> compiler or just memset().
>
> Any better idea to solve the problem in a better way?

Masking off the extra bits, like lower_32_bits()[2] does?

#define POINTER_PATTERN ((void *)(uintptr_t)((ULL_PATTERN) & UINTPTR_MAX))

[1] https://lore.kernel.org/oe-kbuild-all/[email protected]/
[2] https://elixir.bootlin.com/linux/latest/source/include/linux/kernel.h#L82

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2024-01-03 09:45:52

by Qu Wenruo

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()

On 2024/1/3 19:57, Geert Uytterhoeven wrote:
> Hi Qu,
>
> On Tue, Jan 2, 2024 at 9:56 PM Qu Wenruo <[email protected]> wrote:
>> On 2024/1/2 23:53, Geert Uytterhoeven wrote:
>>> On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <[email protected]> wrote:
>>>> The new tests cases for memparse_safe() include:
>>>>
>>>> - The existing test cases for kstrtoull()
>>>> Including all the 3 bases (8, 10, 16), and all the ok and failure
>>>> cases.
>>>> Although there are something we need to verify specific for
>>>> memparse_safe():
>>>>
>>>> * @retptr and @value are not modified for failure cases
>>>>
>>>> * return value are correct for failure cases
>>>>
>>>> * @retptr is correct for the good cases
>>>>
>>>> - New test cases
>>>> Not only testing the result value, but also the @retptr, including:
>>>>
>>>> * good cases with extra tailing chars, but without valid prefix
>>>> The @retptr should point to the first char after a valid string.
>>>> 3 cases for all the 3 bases.
>>>>
>>>> * good cases with extra tailing chars, with valid prefix
>>>> 5 cases for all the suffixes.
>>>>
>>>> * bad cases without any number but stray suffix
>>>> Should be rejected with -EINVAL
>>>>
>>>> Signed-off-by: Qu Wenruo <[email protected]>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/lib/test-kstrtox.c
>>>> +++ b/lib/test-kstrtox.c
>>>> @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void)
>>>> TEST_OK(kstrtoll, long long, "%lld", test_ll_ok);
>>>> }
>>>>
>>>> +/*
>>>> + * The special pattern to make sure the result is not modified for error cases.
>>>> + */
>>>> +#define ULL_PATTERN (0xefefefef7a7a7a7aULL)
>>>> +#if BITS_PER_LONG == 32
>>>> +#define POINTER_PATTERN (0xefef7a7a7aUL)
>>>
>>> This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit
>>> unsigned long or pointer. Probably you wanted to use 0xef7a7a7aUL
>>> instead?
>>
>> My bad, one extra byte...
>
> So did that fix the sparse warning? ;-)

Intel guys have already masked this particular warning.

But your newer suggestion is much better.

>
>>>> +#else
>>>> +#define POINTER_PATTERN (ULL_PATTERN)
>>>> +#endif
>>>
>>> Shouldn't a simple cast to uintptr_t work fine for both 32-bit and
>>> 64-bit systems:
>>>
>>> #define POINTER_PATTERN ((uintptr_t)ULL_PATTERN)
>>>
>>> Or even better, incorporate the cast to a pointer:
>>>
>>> #define POINTER_PATTERN ((void *)(uintptr_t)ULL_PATTERN)
>>
>> The problem is reported by sparse, which warns about that ULL_PATTERN
>> converted to a pointer would lose its width:
>>
>> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from
>> constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
>
> Ah yes, sparse can be annoying.
> I'm still looking for a clean and concise way to shut up [1].
>
>> I'm not sure if using uiintptr_t would solve it, thus I go the macro to
>> switch the value to avoid the static checker's warning.
>>
>> I tried to check how other locations handles patterned pointer value,
>> like CONFIG_INIT_STACK_ALL_PATTERN, but they're either relying on the
>> compiler or just memset().
>>
>> Any better idea to solve the problem in a better way?
>
> Masking off the extra bits, like lower_32_bits()[2] does?
>
> #define POINTER_PATTERN ((void *)(uintptr_t)((ULL_PATTERN) & UINTPTR_MAX))

This sounds much better to me.

I would go this path instead, and finally no need to manually count how
many bytes (which I already failed once).

Thanks,
Qu

>
> [1] https://lore.kernel.org/oe-kbuild-all/[email protected]/
> [2] https://elixir.bootlin.com/linux/latest/source/include/linux/kernel.h#L82
>
> Gr{oetje,eeting}s,
>
> Geert
>

Attachments:

OpenPGP_0xC23D91F3A125FEA8.asc (6.86 kB)
OpenPGP public key OpenPGP_signature.asc (505.00 B)
OpenPGP digital signature Download all attachments

2024-01-03 22:53:18

by Qu Wenruo

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()

On 2024/1/3 00:47, David Disseldorp wrote:
> On Tue, 2 Jan 2024 14:42:13 +1030, Qu Wenruo wrote:
>
[...]
>> + {"P", -EINVAL},
>> +
>> + /* Overflow in the string itself*/
>> + {"18446744073709551616", -ERANGE},
>> + {"02000000000000000000000", -ERANGE},
>> + {"0x10000000000000000", -ERANGE},
> nit: ^ whitespace damage

Sorry, I didn't get the point here.

I checked the patch it's a single space.
Or I missed/screwed up something?

>> +
>> + /*
[...]
>> +
>> + /*
>> + * Finally one suffix then tailing chars, to test the @retptr
>> + * behavior.
>> + */
>> + {"68k ", 69632, 3},
>> + {"8MS", 8388608, 2},
>> + {"0xaeGis", 0x2b80000000, 5},
>> + {"0xaTx", 0xa0000000000, 4},
>> + {"3E8", 0x3000000000000000, 2},
>
> In future it'd be good to get some coverage for non-MEMPARSE_TEST_SUFFIX
> use cases, e.g.:
> /* supported suffix, but not provided with @suffixes */
> {"7K", (MEMPARSE_SUFFIX_M |\
> MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
> MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E), 7, 1},

That's a great idea, since I'm still prepare a v3, it's not hard to add
it into v3.

Thanks,
Qu
>
>> + };
>> + unsigned int i;
>> +
>> + for_each_test(i, tests) {
>> + const struct memparse_test_ok *t = &tests[i];
>> + unsigned long long tmp;
>> + char *retptr;
>> + int ret;
>> +
>> + ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
>> + if (ret != 0) {
>> + WARN(1, "str '%s', expected ret 0 got %d\n", t->str, ret);
>> + continue;
>> + }
>> + if (tmp != t->expected_value)
>> + WARN(1, "str '%s' incorrect result, expected %llu got %llu",
>> + t->str, t->expected_value, tmp);
>> + if (retptr != t->str + t->retptr_off)
>> + WARN(1, "str '%s' incorrect endptr, expected %u got %zu",
>> + t->str, t->retptr_off, retptr - t->str);
>> + }
>> +}
>> static void __init test_kstrtoll_fail(void)
>> {
>> static DEFINE_TEST_FAIL(test_ll_fail) = {
>> @@ -710,6 +941,10 @@ static int __init test_kstrtox_init(void)
>> test_kstrtoll_ok();
>> test_kstrtoll_fail();
>>
>> + test_memparse_safe_ok();
>> + test_memparse_safe_fail();
>> +
>> +
> nit: whitespace ^
>
>> test_kstrtou64_ok();
>> test_kstrtou64_fail();
>> test_kstrtos64_ok();
>
> With Geert's comments addressed:
> Reviewed-by: David Disseldorp <[email protected]>
>