mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

PATCH In review Dan Carpenter 5 participants 7 messages Sep 25, 2019 – Oct 1, 2019

Subsystems (3)

Memory management Kernel janitors MM core

lore

Share & tools

lore

Dan Carpenter

author

2019-09-25 11:04 UTC #0 lore raw

PATCH 1/1 mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

The "mode" and "level" variables are enums and in this context GCC will treat them as unsigned ints so the error handling is never triggered.

I also removed the bogus initializer because it isn't required any more and it's sort of confusing.

Fixes 3cadfa2b9497 ("mm/vmpressure.c: convert to use match_string() helper")

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

---
 mm/vmpressure.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

mm/vmpressure.c +3 -3

index f3b50811497a..fe077500cf20 100644
@@ -362,7 +362,7 @@ int vmpressure_register_event(struct mem_cgroup *memcg,
 	struct vmpressure *vmpr = memcg_to_vmpressure(memcg);
 	struct vmpressure_event *ev;
 	enum vmpressure_modes mode = VMPRESSURE_NO_PASSTHROUGH;
-	enum vmpressure_levels level = -1;
+	enum vmpressure_levels level;
 	char *spec, *spec_orig;
 	char *token;
 	int ret = 0;

@@ -376,7 +376,7 @@ int vmpressure_register_event(struct mem_cgroup *memcg,
 	/* Find required level */
 	token = strsep(&spec, ",");
 	level = match_string(vmpressure_str_levels, VMPRESSURE_NUM_LEVELS, token);
-	if (level < 0) {
+	if ((int)level < 0) {
 		ret = level;
 		goto out;
 	}

@@ -385,7 +385,7 @@ int vmpressure_register_event(struct mem_cgroup *memcg,
 	token = strsep(&spec, ",");
 	if (token) {
 		mode = match_string(vmpressure_str_modes, VMPRESSURE_NUM_MODES, token);
-		if (mode < 0) {
+		if ((int)mode < 0) {
 			ret = mode;
 			goto out;
 		}

Signature · 1 line

2.20.1

Andy Shevchenko

Reviewed-by on v1 1/1

2019-09-25 13:07 UTC #1 lore raw

Quoted 6 lines · Dan Carpenter

The "mode" and "level" variables are enums and in this context GCC will treat them as unsigned ints so the error handling is never triggered.
I also removed the bogus initializer because it isn't required any more and it's sort of confusing.

Indeed, assembly code is different.
Thank you for catching this!

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 40 lines

Fixes 3cadfa2b9497 ("mm/vmpressure.c: convert to use match_string() helper")

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

---
 mm/vmpressure.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

mm/vmpressure.c +3 -3

index f3b50811497a..fe077500cf20 100644
@@ -362,7 +362,7 @@ int vmpressure_register_event(struct mem_cgroup *memcg,
 	struct vmpressure *vmpr = memcg_to_vmpressure(memcg);
 	struct vmpressure_event *ev;
 	enum vmpressure_modes mode = VMPRESSURE_NO_PASSTHROUGH;
-	enum vmpressure_levels level = -1;
+	enum vmpressure_levels level;
 	char *spec, *spec_orig;
 	char *token;
 	int ret = 0;

@@ -376,7 +376,7 @@ int vmpressure_register_event(struct mem_cgroup *memcg,
 	/* Find required level */
 	token = strsep(&spec, ",");
 	level = match_string(vmpressure_str_levels, VMPRESSURE_NUM_LEVELS, token);
-	if (level < 0) {
+	if ((int)level < 0) {
 		ret = level;
 		goto out;
 	}

@@ -385,7 +385,7 @@ int vmpressure_register_event(struct mem_cgroup *memcg,
 	token = strsep(&spec, ",");
 	if (token) {
 		mode = match_string(vmpressure_str_modes, VMPRESSURE_NUM_MODES, token);
-		if (mode < 0) {
+		if ((int)mode < 0) {
 			ret = mode;
 			goto out;
 		}

Signature · 1 line

2.20.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

David Rientjes

Acked-by on v1 1/1

2019-09-26 19:04 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 8 lines · Dan Carpenter

The "mode" and "level" variables are enums and in this context GCC will treat them as unsigned ints so the error handling is never triggered.
I also removed the bogus initializer because it isn't required any more and it's sort of confusing.
Fixes 3cadfa2b9497 ("mm/vmpressure.c: convert to use match_string() helper")
Signed-off-by Dan Carpenter <[email protected]>

Acked-by David Rientjes <[email protected]>

Andrew Morton

maintainer

2019-09-28 21:23 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 6 lines · On Wed, 25 Sep 2019 14:04:49 +0300 Dan Carpenter <[email protected]>…

The "mode" and "level" variables are enums and in this context GCC will treat them as unsigned ints so the error handling is never triggered.
I also removed the bogus initializer because it isn't required any more and it's sort of confusing.

A bit picky of me, but it's an eyesore to assign an int to an enum, then compare the casted enum to an int then copy the enum back to an int.

How about doing it this way? Only copy the int to the enum once we know it's within range?

mm/vmpressure.c renamed from mm/vmpressure.c~mm-vmpressure-fix-a-signedness-bug-in-vmpressure_register_event-fix +6 -8

@@ -375,20 +375,18 @@ int vmpressure_register_event(struct mem
 
 	/* Find required level */
 	token = strsep(&spec, ",");
-	level = match_string(vmpressure_str_levels, VMPRESSURE_NUM_LEVELS, token);
-	if ((int)level < 0) {
-		ret = level;
+	ret = match_string(vmpressure_str_levels, VMPRESSURE_NUM_LEVELS, token);
+	if (ret < 0)
 		goto out;
-	}
+	level = ret;
 
 	/* Find optional mode */
 	token = strsep(&spec, ",");
 	if (token) {
-		mode = match_string(vmpressure_str_modes, VMPRESSURE_NUM_MODES, token);
-		if ((int)mode < 0) {
-			ret = mode;
+		ret = match_string(vmpressure_str_modes, VMPRESSURE_NUM_MODES, token);
+		if (ret < 0)
 			goto out;
-		}
+		mode = ret;
 	}
 
 	ev = kzalloc(sizeof(*ev), GFP_KERNEL);

Matthew Wilcox

Reviewed-by

2019-09-28 21:47 UTC #4 lore raw

Quoted 2 lines · Andrew Morton

How about doing it this way? Only copy the int to the enum once we know it's within range?

This will return a positive integer on success instead of 0. We need:

mutex_unlock(&vmpr->events_lock);
+ ret = 0;
out:

with that,

Reviewed-by Matthew Wilcox (Oracle) <[email protected]>

How about further adding ...

+ * Return: 0 on success, -ENOMEM on memory failure or -EINVAL if @args could
+ * not be parsed.

Andrew Morton

maintainer

2019-09-28 22:04 UTC #5 lore raw

Quoted 10 lines · On Sat, 28 Sep 2019 14:47:02 -0700 Matthew Wilcox <[email protected]> wrote:

Quoted 2 lines · Andrew Morton
How about doing it this way? Only copy the int to the enum once we know it's within range?
This will return a positive integer on success instead of 0. We need:
(quoted diff) +1 -0
 	mutex_unlock(&vmpr->events_lock);
+	ret = 0;
 out:

Quoted 8 lines

with that,
Reviewed-by Matthew Wilcox (Oracle) <[email protected]>
How about further adding ...
+ * Return: 0 on success, -ENOMEM on memory failure or -EINVAL if @args could
+ * not be parsed.

Cool.

mm/vmpressure.c renamed from mm/vmpressure.c~mm-vmpressure-fix-a-signedness-bug-in-vmpressure_register_event-fix-fix +4 -0

@@ -355,6 +355,9 @@ void vmpressure_prio(gfp_t gfp, struct m
  * "hierarchy" or "local").
  *
  * To be used as memcg event method.
+ *
+ * Return: 0 on success, -ENOMEM on memory failure or -EINVAL if @args could
+ * not be parsed.
  */
 int vmpressure_register_event(struct mem_cgroup *memcg,
 			      struct eventfd_ctx *eventfd, const char *args)

@@ -402,6 +405,7 @@ int vmpressure_register_event(struct mem
 	mutex_lock(&vmpr->events_lock);
 	list_add(&ev->node, &vmpr->events);
 	mutex_unlock(&vmpr->events_lock);
+	ret = 0;
 out:
 	kfree(spec_orig);
 	return ret;

Dan Carpenter

author

2019-10-01 07:20 UTC #6 lore raw

In reply to Andrew Morton

Sorry, for not replying. I got sick last week so I was out of office. Feeling better now.

regards,
dan carpenter