On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.ha...@samsung.com> wrote:
> Current implementation of IS_ERR_VALUE works correctly only with > following types: > - unsigned long, > - short, int, long. > Other types are handled incorrectly either on 32-bit either on 64-bit > either on both architectures. > The patch fixes it by comparing argument with MAX_ERRNO casted > to argument's type for unsigned types and comparing with zero for signed > types. As a result all integer types bigger than char are handled properly. > > I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35 > cases it is used incorrectly, ie it can hide errors depending of 32/64 bit > architecture. Instead of fixing usage I propose to enhance the macro > to cover more types. > And just for the record: the macro is used 101 times with signed variables, > I am not sure if it should be preferred over simple comparison "ret < 0", > but the new version can do it as well. > > And below list of detected potential errors: > > ... > > --- a/include/linux/err.h > +++ b/include/linux/err.h > @@ -18,7 +18,9 @@ > > #ifndef __ASSEMBLY__ > > -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO) > +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \ > + ? unlikely((x) < 0) \ > + : unlikely((x) >= (typeof(x))-MAX_ERRNO)) > hm, seems complicated. Can we simply cast the value to long? #define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO) and simplify that to #define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO) or something like that.
