On Sun, Jul 08, 2018 at 03:31:45PM +0000, Bart Van Assche wrote: > On Sun, 2018-07-08 at 13:38 +0300, Leon Romanovsky wrote: > > +/* > > + * 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 > > If s >= sizeof(a) * 8 then a << s triggers undefined behavior. There is no guarantee > that the result will be 0. See also > http://blog.llvm.org/2011/05/what-every-c-programmer-should-know_21.html. This is already prevented with this: + unsigned int _to_shift = \ + _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \ And 'a' is promoted to a u64 before shifting so there is no 'd' larger than 8 bytes. > > + * - 'a' is negative > > What is wrong with using (a << s) if a < 0? The invariant of this macros is that it never produces a negative result. This is because it is designed to be a 'security' protection for unsigned shifts, not something trying to be general. I don't think it makes sense to mix in signeded support since that doesn't seem to actually be a useful thing. If someone wants that then they should make another macro and figure out what the semantics are.. > > + * - 'a << s' sets the sign bit, if any, in '*d' > > Setting the highest bit is fine if a is unsigned. No, it isn't, it makes 'd' negative, which is to be considered an overflow. The definition of the macro is to compute unsigned a << s on infinite precision and fail if d != (a << s). This is why the type of 'd' is necessary... > > + * *d is not defined if false is returned. > > + */ > > +#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; \ > > + (_to_shift != _s || *_d < 0 || _a < 0 || \ > > + (*_d >> _to_shift) != _a); \ > > +}) > > I think the fact that the above macro stores the result in a pointer passed > as argument will reduce readability. How about the macro below, which > addresses all the shortcomings mentioned above? We can't protect against overflow into 'd' if we don't know the type of 'd', so the pointer output is mandatory. This is also the standard pattern for everything in overflow.h. Jason -- 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
