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

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

[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 <dan.carpenter@oracle.com>
---
 mm/vmpressure.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/vmpressure.c b/mm/vmpressure.c
index f3b50811497a..fe077500cf20 100644
--- a/mm/vmpressure.c
+++ b/mm/vmpressure.c
@@ -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;
 		}
-- 
2.20.1

Re: [PATCH] mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

[Andy Shevchenko]

On Wed, Sep 25, 2019 at 02:04:49PM +0300, Dan Carpenter wrote:
> 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 <andriy.shevchenko@linux.intel.com>

> Fixes: 3cadfa2b9497 ("mm/vmpressure.c: convert to use match_string() helper")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  mm/vmpressure.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> index f3b50811497a..fe077500cf20 100644
> --- a/mm/vmpressure.c
> +++ b/mm/vmpressure.c
> @@ -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;
>  		}
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

[David Rientjes]

On Wed, 25 Sep 2019, Dan Carpenter wrote:

> 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 <dan.carpenter@oracle.com>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

[Andrew Morton]

On Wed, 25 Sep 2019 14:04:49 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> 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?

--- a/mm/vmpressure.c~mm-vmpressure-fix-a-signedness-bug-in-vmpressure_register_event-fix
+++ a/mm/vmpressure.c
@@ -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);
_

Re: [PATCH] mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

[Matthew Wilcox]

On Sat, Sep 28, 2019 at 02:23:56PM -0700, Andrew Morton wrote:
> 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) <willy@infradead.org>

How about further adding ...

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

Re: [PATCH] mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

[Andrew Morton]

On Sat, 28 Sep 2019 14:47:02 -0700 Matthew Wilcox <willy@infradead.org> wrote:

> On Sat, Sep 28, 2019 at 02:23:56PM -0700, Andrew Morton wrote:
> > 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) <willy@infradead.org>
> 
> How about further adding ...
> 
> + * Return: 0 on success, -ENOMEM on memory failure or -EINVAL if @args could
> + * not be parsed.

Cool.

--- a/mm/vmpressure.c~mm-vmpressure-fix-a-signedness-bug-in-vmpressure_register_event-fix-fix
+++ a/mm/vmpressure.c
@@ -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;
_

Re: [PATCH] mm, vmpressure: Fix a signedness bug in vmpressure_register_event()

[Dan Carpenter]

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

regards,
dan carpenter