| Message ID | 20180801000039.44314-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | overflow.h: Add arithmetic shift helper | expand |
On 07/31/2018 05:00 PM, Kees Cook wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > Add shift_overflow() helper to assist driver authors in ensuring that > shift operations don't cause overflows or other odd conditions. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > [kees: tweaked comments and commit log, dropped unneeded assignment] > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/overflow.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index 8712ff70995f..69fc366ce865 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -202,6 +202,37 @@ > > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ > > +/** check_shift_overflow() - Calculate a left-shifted value and check overflow > + * > + * @a: Value to be shifted > + * @b: How many bits left to shift > + * @d: Pointer to where to store the result > + * > + * Computes *@d = (@a << @s) @b) > + * > + * Returns true if '*d' cannot hold the result or when 'a << s' doesn't > + * make sense. Example conditions: > + * - 'a << s' causes bits to be lost when stored in *d. > + * - 's' is garbage (e.g. negative) or so large that the result of > + * 'a << s' is guaranteed to be 0. > + * - 'a' is negative. > + * - 'a << s' sets the sign bit, if any, in '*d'. > + * > + * '*d' will hold the results of the attempted shift, but is not > + * considered "safe for use" 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); \ > + (_to_shift != _s || *_d < 0 || _a < 0 || \ > + (*_d >> _to_shift) != _a); \ > +}) > + > /** > * array_size() - Calculate size of 2-dimensional array. > * >
On Tue, Jul 31, 2018 at 05:00:37PM -0700, Kees Cook wrote: > From: Jason Gunthorpe <jgg@mellanox.com> > > Add shift_overflow() helper to assist driver authors in ensuring that > shift operations don't cause overflows or other odd conditions. > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > [kees: tweaked comments and commit log, dropped unneeded assignment] > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/overflow.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index 8712ff70995f..69fc366ce865 100644 > --- a/include/linux/overflow.h > +++ b/include/linux/overflow.h > @@ -202,6 +202,37 @@ > > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ > > +/** check_shift_overflow() - Calculate a left-shifted value and check overflow > + * > + * @a: Value to be shifted > + * @b: How many bits left to shift The above @b should be @s > +#define check_shift_overflow(a, s, d) ({ \ Should I run this series through the rdma tree? Thanks, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 31, 2018 at 7:15 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Tue, Jul 31, 2018 at 05:00:37PM -0700, Kees Cook wrote: >> From: Jason Gunthorpe <jgg@mellanox.com> >> >> Add shift_overflow() helper to assist driver authors in ensuring that >> shift operations don't cause overflows or other odd conditions. >> >> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> >> Signed-off-by: Leon Romanovsky <leonro@mellanox.com> >> [kees: tweaked comments and commit log, dropped unneeded assignment] >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> include/linux/overflow.h | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >> index 8712ff70995f..69fc366ce865 100644 >> --- a/include/linux/overflow.h >> +++ b/include/linux/overflow.h >> @@ -202,6 +202,37 @@ >> >> #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ >> >> +/** check_shift_overflow() - Calculate a left-shifted value and check overflow >> + * >> + * @a: Value to be shifted >> + * @b: How many bits left to shift > > The above @b should be @s Ooops, copy-paste-o. :) > >> +#define check_shift_overflow(a, s, d) ({ \ > > Should I run this series through the rdma tree? I'd like to get Rasmus's Ack, but otherwise, yes, that'd be fine. -Kees
The idea is nice, but I don't like the API. The "_overflow" feels too
specific because maybe we could check for other things in the future.
Normally boolean macros should say they are boolean in the name and I
would prefer if it returned zero on failure.
if (!checked_shift(dest, mask, shift)) {
if (!shift_ok(dest, mask, shift)) {
if (!safe_shift(dest, mask, shift)) {
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 01, 2018 at 10:57:44AM +0300, Dan Carpenter wrote: > The idea is nice, but I don't like the API. The "_overflow" feels too > specific because maybe we could check for other things in the future. > Normally boolean macros should say they are boolean in the name and I > would prefer if it returned zero on failure. > > if (!checked_shift(dest, mask, shift)) { > if (!shift_ok(dest, mask, shift)) { > if (!safe_shift(dest, mask, shift)) { Huh... It turns out I put the argument order different as well. If we wanted to keep it returning 1 on failure then some other names are: if (shift_failed(dest, mask, shift)) { if (shift_error(dest, mask, shift)) { if (shift_overflow(dest, mask, shift)) { regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 1, 2018 at 1:07 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Wed, Aug 01, 2018 at 10:57:44AM +0300, Dan Carpenter wrote: >> The idea is nice, but I don't like the API. The "_overflow" feels too >> specific because maybe we could check for other things in the future. >> Normally boolean macros should say they are boolean in the name and I >> would prefer if it returned zero on failure. >> >> if (!checked_shift(dest, mask, shift)) { >> if (!shift_ok(dest, mask, shift)) { >> if (!safe_shift(dest, mask, shift)) { > > Huh... It turns out I put the argument order different as well. > > If we wanted to keep it returning 1 on failure then some other names > are: > > if (shift_failed(dest, mask, shift)) { > if (shift_error(dest, mask, shift)) { > if (shift_overflow(dest, mask, shift)) { This is following the existing check_{add,mul}_overflow() helpers, which are based on the gcc helpers. I'd like to keep things consistent. -Kees
On Wed, Aug 01, 2018 at 11:07:24AM +0300, Dan Carpenter wrote: > On Wed, Aug 01, 2018 at 10:57:44AM +0300, Dan Carpenter wrote: > > The idea is nice, but I don't like the API. The "_overflow" feels too > > specific because maybe we could check for other things in the future. > > Normally boolean macros should say they are boolean in the name and I > > would prefer if it returned zero on failure. > > > > if (!checked_shift(dest, mask, shift)) { > > if (!shift_ok(dest, mask, shift)) { > > if (!safe_shift(dest, mask, shift)) { > > Huh... It turns out I put the argument order different as well. > > If we wanted to keep it returning 1 on failure then some other names > are: > > if (shift_failed(dest, mask, shift)) { > if (shift_error(dest, mask, shift)) { > if (shift_overflow(dest, mask, shift)) { I think this ship has sailed, the convention for these tests is already established in overflow.h. ie: check_add_overflow check_sub_overflow check_mul_overflow Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/overflow.h b/include/linux/overflow.h index 8712ff70995f..69fc366ce865 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -202,6 +202,37 @@ #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ +/** check_shift_overflow() - Calculate a left-shifted value and check overflow + * + * @a: Value to be shifted + * @b: How many bits left to shift + * @d: Pointer to where to store the result + * + * Computes *@d = (@a << @s) + * + * Returns true if '*d' cannot hold the result or when 'a << s' doesn't + * make sense. Example conditions: + * - 'a << s' causes bits to be lost when stored in *d. + * - 's' is garbage (e.g. negative) or so large that the result of + * 'a << s' is guaranteed to be 0. + * - 'a' is negative. + * - 'a << s' sets the sign bit, if any, in '*d'. + * + * '*d' will hold the results of the attempted shift, but is not + * considered "safe for use" 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); \ + (_to_shift != _s || *_d < 0 || _a < 0 || \ + (*_d >> _to_shift) != _a); \ +}) + /** * array_size() - Calculate size of 2-dimensional array. *