On Tue, Jul 10, 2018 at 08:09:32AM +0200, Rasmus Villemoes wrote:
> On 2018-07-08 12:38, Leon Romanovsky wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> >
> > Add shift_overflow() helper to help driver authors to ensure that
> > shift operand doesn't cause to overflow.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > ---
> > include/linux/overflow.h | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> > index 8712ff70995f..21ff032773e0 100644
> > --- a/include/linux/overflow.h
> > +++ b/include/linux/overflow.h
> > @@ -202,6 +202,29 @@
> >
> > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */
> >
> > +/*
> > + * Compute *d = (a << s)
> > + *
> > + * Returns true if '*d' cannot hold the result or 'a << s' doesn't make sense.
> > + * - 'a << s' causes bits to be lost when stored in d
> > + * - 's' is garbage (eg negative) or so large that a << s is guaranteed to be 0
> > + * - 'a' is negative
> > + * - 'a << s' sets the sign bit, if any, in '*d'
> > + * *d is not defined if false is returned.
> > + */
>
> You mean if true is returned, i.e. some kind of overflow or nonsense
> input was detected. *d is very much defined if the result is false.
>
> > +#define check_shift_overflow(a, s, d) ({ \
> > + typeof(a) _a = a; \
> > + typeof(s) _s = s; \
> > + typeof(d) _d = d; \
> > + u64 _a_full = _a; \
> > + unsigned int _to_shift = \
> > + _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \
> > + *_d = (_a_full << _to_shift); \
> > + *d = *_d; \
>
> As others have pointed out, this assignment is useless at best, but
> potentially harmful - the whole point of the standard typeof dance is to
> prevent double evaluation of macro arguments...
In the lib/test_overflow.c, there is expectation what result of
check_one_op will be defined anyway (line 231). I wanted a way to have
overflowed result placed in *d for tests, but didn't want for users rely
on that, so this is why comment says that the result is unreliable.
220 #define check_one_op(t, fmt, op, sym, a, b, r, of) do { \
221 t _r; \
222 bool _of; \
223 \
224 _of = check_ ## op ## _overflow(a, b, &_r); \
225 if (_of != of) { \
226 pr_warn("expected "fmt" "sym" "fmt \
227 " to%s overflow (type %s)\n", \
228 a, b, of ? "" : " not", #t); \
229 err = 1; \
230 } \
231 if (_r != r) { \
232 pr_warn("expected "fmt" "sym" "fmt" == " \
233 fmt", got "fmt" (type %s)\n", \
234 a, b, r, _r, #t); \
235 err = 1; \
236 } \
237 } while (0)
So or we are getting rid from *d assignment in check_shift_overflow()
macro and changing the test suite to skip line 231 or define *d, like I
did.
Thanks
>
> Rasmus
Attachment:
signature.asc
Description: PGP signature
