lib/test_kmod: Fix an integer overflow test

PATCH In review Dan Carpenter 2 participants 5 messages Jan 22, 2018 – Feb 24, 2018

Subsystems (3)

Kernel janitors Library code Module support

lore

Share & tools

lore

Dan Carpenter

author

2018-01-22 10:27 UTC #0 lore raw

PATCH 1/1 lib/test_kmod: Fix an integer overflow test

The main problem is that the parentheses are in the wrong place and the unlikely() call returns either 0 or 1 so it's never less than zero. The other problem is that signed integer overflows like "INT_MAX + 1" are undefined behavior.

Fixes d9c6a72d6fa2 ("kmod: add test driver to stress test the module loader")

Signed-off-by Dan Carpenter <[email protected]>

lib/test_kmod.c +1 -1

index e372b97eee13..30fd6d9e5361 100644
@@ -1141,7 +1141,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
 	mutex_lock(&reg_dev_mutex);
 
 	/* int should suffice for number of devices, test for wrap */
-	if (unlikely(num_test_devs + 1) < 0) {
+	if (num_test_devs = INT_MAX) {
 		pr_err("reached limit of number of test devices\n");
 		goto out;
 	}

Luis R. Rodriguez

maintainer

2018-02-24 02:59 UTC #1 lore raw

Quoted 2 lines · Dan Carpenter

The main problem is that the parentheses are in the wrong place and the unlikely() call returns either 0 or 1 so it's never less than zero.

Doh, thanks, yes. Seems worth considering a grammar rule for it.

Quoted 2 lines

The other problem is that signed integer overflows like "INT_MAX + 1" are undefined behavior.

Likewise.

This seems like another possible generic typo issue. But I would not resolve it the way you did, in this particular case below num_test_devs represents the number of already registered devs, before we increment. So the way to resolve this would be:

	if (num_test_devs + 1 == INT_MAX)

I'll get this upstream, thanks!

Luis

Quoted 17 lines

Fixes d9c6a72d6fa2 ("kmod: add test driver to stress test the module loader")

Signed-off-by Dan Carpenter <[email protected]>

lib/test_kmod.c +1 -1

index e372b97eee13..30fd6d9e5361 100644
@@ -1141,7 +1141,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
 	mutex_lock(&reg_dev_mutex);
 
 	/* int should suffice for number of devices, test for wrap */
-	if (unlikely(num_test_devs + 1) < 0) {
+	if (num_test_devs == INT_MAX) {
 		pr_err("reached limit of number of test devices\n");
 		goto out;
 	}

Signature · 2 lines

Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg

Dan Carpenter

author

2018-02-24 08:45 UTC #2 lore raw

Quoted 11 lines · Luis R. Rodriguez

Quoted 2 lines · Dan Carpenter
The main problem is that the parentheses are in the wrong place and the unlikely() call returns either 0 or 1 so it's never less than zero.
Doh, thanks, yes. Seems worth considering a grammar rule for it.
Quoted 2 lines
The other problem is that signed integer overflows like "INT_MAX + 1" are undefined behavior.
Likewise.

Quoted 8 lines

This seems like another possible generic typo issue. But I would not resolve it the way you did, in this particular case below num_test_devs represents the number of already registered devs, before we increment. So the way to resolve this would be:
	if (num_test_devs + 1 == INT_MAX)
I'll get this upstream, thanks!

There is no issue if num_test_devs is INT_MAX. But capping it at INT_MAX - 1 is also fine.

regards,
dan carpenter

Luis R. Rodriguez

maintainer

2018-02-24 22:06 UTC #3 lore raw

Quoted 17 lines · Dan Carpenter

Quoted 16 lines · Luis R. Rodriguez
Quoted 2 lines · Dan Carpenter
The main problem is that the parentheses are in the wrong place and the unlikely() call returns either 0 or 1 so it's never less than zero.
Doh, thanks, yes. Seems worth considering a grammar rule for it.
Quoted 2 lines
The other problem is that signed integer overflows like "INT_MAX + 1" are undefined behavior.
Likewise.
This seems like another possible generic typo issue. But I would not resolve it the way you did, in this particular case below num_test_devs represents the number of already registered devs, before we increment. So the way to resolve this would be:

Quoted 6 lines

Quoted 3 lines
	if (num_test_devs + 1 == INT_MAX)
I'll get this upstream, thanks!
There is no issue if num_test_devs is INT_MAX. But capping it at INT_MAX - 1 is also fine.

If num_test_devs is INT_MAX, then doing num_test_devs + 1 overflows and as you noted that is undefined?

Luis

Dan Carpenter

author

2018-02-24 23:34 UTC #4 lore raw

Quoted 20 lines · Luis R. Rodriguez

Quoted 19 lines · Dan Carpenter
Quoted 18 lines · Luis R. Rodriguez
Quoted 2 lines · Dan Carpenter
The main problem is that the parentheses are in the wrong place and the unlikely() call returns either 0 or 1 so it's never less than zero.
Doh, thanks, yes. Seems worth considering a grammar rule for it.
Quoted 2 lines
The other problem is that signed integer overflows like "INT_MAX + 1" are undefined behavior.
Likewise.
This seems like another possible generic typo issue. But I would not resolve it the way you did, in this particular case below num_test_devs represents the number of already registered devs, before we increment. So the way to resolve this would be:
	if (num_test_devs + 1 == INT_MAX)

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
I'll get this upstream, thanks!
There is no issue if num_test_devs is INT_MAX. But capping it at INT_MAX - 1 is also fine.
If num_test_devs is INT_MAX, then doing num_test_devs + 1 overflows and as you noted that is undefined?

If it's INT_MAX we never do "num_test_devs + 1", we return a NULL.

regards,
dan carpenter