On 2018-07-09 19:57, Kees Cook wrote:
> On Sun, Jul 8, 2018 at 3:38 AM, Leon Romanovsky <leon@xxxxxxxxxx>
> wrote:
>> From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>>
>> Expand existing check_*_overflow tests to support
>> check_shift_overflow().
>>
>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>
> This is a painful diff due to the new field
Yeah, and that's not the right way to do these tests. Unlike +, - and *,
a and s do not play a symmetrical role in a<<s, so it doesn't make sense
to try to shoehorn the tests into those tables, not to mention that most
of the existing b values are obviously garbage when used as s. Moreover,
by the nature of those tables and the decision to force type hygiene for
the existing check_*_overflow, there are no tests where typeof(a) and
typeof(*d) have different signedness, which I specifically asked for.
And, if we really don't want to define the result in case of some kind
of overflow (which I don't like, but OTOH agree that it's hard to define
sensible semantics), the test cases should not enshrine some specific
result - whatever the implementation happens to produce currently - that
makes it harder to change the implementation. This also makes them unfit
for the existing scheme.
In short, I strongly disagree with the cover letter's "but good enough".
So please, do test the mixed-types and in particular mixed-signedness,
and the corner cases of "the shift is all right, but then we do a
narrowing store", or "a is 0, but still catch bogus shift counts".
Something like this with lots of more test cases (and sorry if this
turns out whitespace damaged)
static int __init test_shift_overflow(void)
{
int err = 0;
#define TEST_ONE_SHIFT(a, s, t, expect, of) do { \
typeof(a) __a = (a); \
typeof(s) __s = (s); \
t __e = (expect); \
t __d; \
bool __of = check_shift_overflow(__a, __s, &__d); \
if (__of != of) { \
pr_warn("expected %s << %s to%s overflow\n", \
#a, #s, of ? "" : " not"); \
err = 1; \
} \
if (!__of && __d != __e) { \
pr_warn("expected (%s)(%s << %s) == %s\n", \
#t, #a, #s, #expect); \
if ((t)-1 < 0) \
pr_warn("got %lld\n", (s64)__d); \
else \
pr_warn("got %llu\n", (u64)__d); \
err = 1; \
} \
} while (0)
TEST_ONE_SHIFT(1, 30, int, 1<<30, false);
TEST_ONE_SHIFT(1, 31, int, 0, true);
TEST_ONE_SHIFT(1, 31, unsigned, 1U<<31, false);
TEST_ONE_SHIFT(1, 40, u64, 1ULL<<40, false);
TEST_ONE_SHIFT(150, 1, u8, 0, true);
TEST_ONE_SHIFT(-123, 0, int, 0, true);
TEST_ONE_SHIFT(0, 31, u32, 0, false);
TEST_ONE_SHIFT(0, 32, u32, 0, true);
TEST_ONE_SHIFT(0, -5, u32, 0, true);
return err;
}
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
