Skip to contentSkip to conversation

XArray: Fix a math problem in xa_is_err()

PATCH In review Dan Carpenter 2 participants 3 messages Jan 15, 2019

Subsystems (3)
VFS Kernel janitors Xarray
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 XArray: Fix a math problem in xa_is_err()

There is a math problem here which leads to a lot of static checker warnings for me:

net/sunrpc/clnt.c:451 rpc_new_client() error: (-4096) too low for ERR_PTR

Error values are from -1 to -4095 or from 0xffffffff to 0xfffff001 in hexadecimal. (I am assuming a 32 bit system for simplicity). We are using the lowest two bits to hold some internal XArray data so the error is shifted two spaces to the left. 0xfffff001 << 2 is 0xffffc004. And finally we want to check that BIT(1) is set so we add 2 which gives us 0xffffc006.

In other words, we should be checking that "entry >= 0xffffc006", but the check is actually testing if "entry >= 0xffffc002".

Fixes 76b4e5299565 ("XArray: Permit storing 2-byte-aligned pointers")
Signed-off-by Dan Carpenter <[email protected]>
---
 include/linux/xarray.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/xarray.h +1 -1
index 12244aa98a69..4208042f939a 100644
@@ -177,7 +177,8 @@ static inline bool xa_is_internal(const void *entry)
 static inline bool xa_is_err(const void *entry)
 {
 	return unlikely(xa_is_internal(entry) &&
-			(unsigned long)entry >= -((MAX_ERRNO << 2) + 2));
+			(unsigned long)entry >+				(((unsigned long)(-MAX_ERRNO << 2) + 2)));
 }
 
 /**
Signature · 1 line
2.17.1
maintainer
#1 lore raw

Review static inline bool xa_is_internal(const void *entry) · line 181

Quoted 7 lines · Dan Carpenter
(quoted diff) +1 -1
@@ -177,7 +177,8 @@ static inline bool xa_is_internal(const void *entry)
 static inline bool xa_is_err(const void *entry)
 {
 	return unlikely(xa_is_internal(entry) &&
-			(unsigned long)entry >= -((MAX_ERRNO << 2) + 2));
+			(unsigned long)entry >> +				(((unsigned long)(-MAX_ERRNO << 2) + 2)));
 }

Ugh all the brackets, I'm not surprised I got it wrong. How about this instead; does it make your static checker happy?

include/linux/xarray.h +1 -1
index 7da665f5cb20..5d9d318bcf7a 100644
@@ -177,7 +177,7 @@ static inline bool xa_is_internal(const void *entry)
 static inline bool xa_is_err(const void *entry)
 {
        return unlikely(xa_is_internal(entry) &&
-                       (unsigned long)entry >= -((MAX_ERRNO << 2) + 2));
+                       entry >= xa_mk_internal(-MAX_ERRNO));
 }
 
 /**

(passes sparse & gcc, but ...)

author
#2 lore raw
Quoted 12 lines · Matthew Wilcox

include/linux/xarray.h static inline bool xa_is_internal(const void *entry) · line 181

Quoted 7 lines · Dan Carpenter
include/linux/xarray.h +1 -1
@@ -177,7 +177,8 @@ static inline bool xa_is_internal(const void *entry)
 static inline bool xa_is_err(const void *entry)
 {
 	return unlikely(xa_is_internal(entry) &&
-			(unsigned long)entry >= -((MAX_ERRNO << 2) + 2));
+			(unsigned long)entry >> > +				(((unsigned long)(-MAX_ERRNO << 2) + 2)));
 }

Ugh all the brackets, I'm not surprised I got it wrong. How about this instead; does it make your static checker happy?

I wasn't a super fan of that expression either... I think your code will work, and if not then it's the checker which needs fixing. Thanks!

regards,
dan carpenter