Skip to contentSkip to conversation

gru: fix a sanity test in gru_set_context_option()

PATCH In review Dan Carpenter 2 participants 5 messages May 19, 2013 – May 20, 2013

Subsystems (2)
Kernel janitors Char and misc drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 gru: fix a sanity test in gru_set_context_option()

"req.val1 = -1" is valid but it doesn't make sense to check gru_base[-1]. gru_base[] is a global array.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/sgi-gru/grufault.c +2 -1
index c4acac7..e7c37b2 100644
@@ -876,7 +876,8 @@ int gru_set_context_option(unsigned long arg)
 	switch (req.op) {
 	case sco_blade_chiplet:
 		/* Select blade/chiplet for GRU context */
-		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES || !gru_base[req.val1] ||
+		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES ||
+		    (req.val1 != -1 && !gru_base[req.val1]) ||
 		    req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB) {
 			ret = -EINVAL;
 		} else {
#1 lore raw
Quoted 10 lines · Dan Carpenter

"req.val1 = -1" is valid but it doesn't make sense to check gru_base[-1]. gru_base[] is a global array.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/sgi-gru/grufault.c +0 -0
index c4acac7..e7c37b2 100644

drivers/misc/sgi-gru/grufault.c int gru_set_context_option(unsigned long arg) · line 883

Quoted 9 lines
drivers/misc/sgi-gru/grufault.c +2 -1
@@ -876,7 +876,8 @@ int gru_set_context_option(unsigned long arg)
 	switch (req.op) {
 	case sco_blade_chiplet:
 		/* Select blade/chiplet for GRU context */
-		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES || !gru_base[req.val1] ||
+		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES ||
+		    (req.val1 != -1 && !gru_base[req.val1]) ||
 		    req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB) {
 			ret = -EINVAL;
 		} else {

Since we're here, maybe we should neaten this up a little?

Signed-off-by Dimitri Sivanich <[email protected]>
---
 drivers/misc/sgi-gru/grufault.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux/drivers/misc/sgi-gru/grufault.c =================================--- linux.orig/drivers/misc/sgi-gru/grufault.c +++ linux/drivers/misc/sgi-gru/grufault.c
@@ -876,8 +876,9 @@ int gru_set_context_option(unsigned long
switch (req.op) {
case sco_blade_chiplet:
/* Select blade/chiplet for GRU context */

-		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES || !gru_base[req.val1] ||
-		    req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB) {
+		if (req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB ||

+ req.val1 < -1 || req.val1 >= GRU_MAX_BLADES ||
+ (req.val1 >= 0 && !gru_base[req.val1])) {
ret = -EINVAL;
} else {
gts->ts_user_blade_id = req.val1;

author
#2 lore raw
Quoted 12 lines · Dimitri Sivanich

Since we're here, maybe we should neaten this up a little?

Signed-off-by Dimitri Sivanich <[email protected]>
---
 drivers/misc/sgi-gru/grufault.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux/drivers/misc/sgi-gru/grufault.c =================================> --- linux.orig/drivers/misc/sgi-gru/grufault.c +++ linux/drivers/misc/sgi-gru/grufault.c

Review int gru_set_context_option(unsigned long · line 882

Quoted 9 lines
(quoted diff) +3 -2
@@ -876,8 +876,9 @@ int gru_set_context_option(unsigned long
 	switch (req.op) {
 	case sco_blade_chiplet:
 		/* Select blade/chiplet for GRU context */
-		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES || !gru_base[req.val1] ||
-		    req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB) {
+		if (req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB ||
+			req.val1 < -1 || req.val1 >= GRU_MAX_BLADES ||
+			(req.val1 >= 0 && !gru_base[req.val1])) {
 			ret = -EINVAL;

I generally prefer how in the original code all the sub conditions were aligned nicely but I will Ack your version as well. Thanks.

regards,
dan carpenter

Quoted 2 lines
(quoted diff) +0 -0
 		} else {
 			gts->ts_user_blade_id = req.val1;
#3 lore raw
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Dimitri Sivanich

Since we're here, maybe we should neaten this up a little?

Signed-off-by Dimitri Sivanich <[email protected]>
---
 drivers/misc/sgi-gru/grufault.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Quoted 17 lines

Review int gru_set_context_option(unsigned long · line 882

Quoted 13 lines

Index: linux/drivers/misc/sgi-gru/grufault.c =================================> > --- linux.orig/drivers/misc/sgi-gru/grufault.c +++ linux/drivers/misc/sgi-gru/grufault.c

(quoted diff) +3 -2
@@ -876,8 +876,9 @@ int gru_set_context_option(unsigned long
 	switch (req.op) {
 	case sco_blade_chiplet:
 		/* Select blade/chiplet for GRU context */
-		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES || !gru_base[req.val1] ||
-		    req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB) {
+		if (req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB ||
+			req.val1 < -1 || req.val1 >= GRU_MAX_BLADES ||
+			(req.val1 >= 0 && !gru_base[req.val1])) {
 			ret = -EINVAL;

I generally prefer how in the original code all the sub conditions were aligned nicely but I will Ack your version as well. Thanks.

I guess I'd have to agree. How about this?

Signed-off-by Dimitri Sivanich <[email protected]>
---
 drivers/misc/sgi-gru/grufault.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux/drivers/misc/sgi-gru/grufault.c =================================--- linux.orig/drivers/misc/sgi-gru/grufault.c +++ linux/drivers/misc/sgi-gru/grufault.c
@@ -876,8 +876,9 @@ int gru_set_context_option(unsigned long
switch (req.op) {
case sco_blade_chiplet:
/* Select blade/chiplet for GRU context */

-		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES || !gru_base[req.val1] ||
-		    req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB) {
+		if (req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB ||
+		    req.val1 < -1 || req.val1 >= GRU_MAX_BLADES ||

+ (req.val1 >= 0 && !gru_base[req.val1])) {
ret = -EINVAL;
} else {
gts->ts_user_blade_id = req.val1;

author Acked-by
#4 lore raw
Quoted 28 lines · Dimitri Sivanich
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Dimitri Sivanich

Since we're here, maybe we should neaten this up a little?

Signed-off-by Dimitri Sivanich <[email protected]>
---
 drivers/misc/sgi-gru/grufault.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Quoted 17 lines

Review int gru_set_context_option(unsigned long · line 882

Quoted 13 lines

Index: linux/drivers/misc/sgi-gru/grufault.c =================================> > > --- linux.orig/drivers/misc/sgi-gru/grufault.c +++ linux/drivers/misc/sgi-gru/grufault.c

(quoted diff) +3 -2
@@ -876,8 +876,9 @@ int gru_set_context_option(unsigned long
 	switch (req.op) {
 	case sco_blade_chiplet:
 		/* Select blade/chiplet for GRU context */
-		if (req.val1 < -1 || req.val1 >= GRU_MAX_BLADES || !gru_base[req.val1] ||
-		    req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB) {
+		if (req.val0 < -1 || req.val0 >= GRU_CHIPLETS_PER_HUB ||
+			req.val1 < -1 || req.val1 >= GRU_MAX_BLADES ||
+			(req.val1 >= 0 && !gru_base[req.val1])) {
 			ret = -EINVAL;

I generally prefer how in the original code all the sub conditions were aligned nicely but I will Ack your version as well. Thanks.

Quoted 1 line

I guess I'd have to agree. How about this?

Looks nice. Starting with zero is obviously better than the original.

Acked-by Dan Carpenter <[email protected]>

regards,
dan carpenter