[PATCH v4] kasan/quarantine: fix bugs on qlist_move_cache()

[PATCH v4] kasan/quarantine: fix bugs on qlist_move_cache()

[js1304]

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

There are two bugs on qlist_move_cache(). One is that qlist's tail
isn't set properly. curr->next can be NULL since it is singly linked
list and NULL value on tail is invalid if there is one item on qlist.
Another one is that if cache is matched, qlist_put() is called and
it will set curr->next to NULL. It would cause to stop the loop
prematurely.

These problems come from complicated implementation so I'd like to
re-implement it completely. Implementation in this patch is really
simple. Iterate all qlist_nodes and put them to appropriate list.

Unfortunately, I got this bug sometime ago and lose oops message.
But, the bug looks trivial and no need to attach oops.

v4: fix cache size bug s/cache->size/obj_cache->size/
v3: fix build warning

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 mm/kasan/quarantine.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
index 4973505..b2e1827 100644
--- a/mm/kasan/quarantine.c
+++ b/mm/kasan/quarantine.c
@@ -238,30 +238,23 @@ static void qlist_move_cache(struct qlist_head *from,
 				   struct qlist_head *to,
 				   struct kmem_cache *cache)
 {
-	struct qlist_node *prev = NULL, *curr;
+	struct qlist_node *curr;
 
 	if (unlikely(qlist_empty(from)))
 		return;
 
 	curr = from->head;
+	qlist_init(from);
 	while (curr) {
 		struct qlist_node *qlink = curr;
 		struct kmem_cache *obj_cache = qlink_to_cache(qlink);
 
-		if (obj_cache == cache) {
-			if (unlikely(from->head == qlink)) {
-				from->head = curr->next;
-				prev = curr;
-			} else
-				prev->next = curr->next;
-			if (unlikely(from->tail == qlink))
-				from->tail = curr->next;
-			from->bytes -= cache->size;
-			qlist_put(to, qlink, cache->size);
-		} else {
-			prev = curr;
-		}
 		curr = curr->next;
+
+		if (obj_cache == cache)
+			qlist_put(to, qlink, obj_cache->size);
+		else
+			qlist_put(from, qlink, obj_cache->size);
 	}
 }
 
-- 
1.9.1

Re: [PATCH v4] kasan/quarantine: fix bugs on qlist_move_cache()

[Andrey Ryabinin]

On 07/04/2016 07:31 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> There are two bugs on qlist_move_cache(). One is that qlist's tail
> isn't set properly. curr->next can be NULL since it is singly linked
> list and NULL value on tail is invalid if there is one item on qlist.
> Another one is that if cache is matched, qlist_put() is called and
> it will set curr->next to NULL. It would cause to stop the loop
> prematurely.
> 
> These problems come from complicated implementation so I'd like to
> re-implement it completely. Implementation in this patch is really
> simple. Iterate all qlist_nodes and put them to appropriate list.
> 
> Unfortunately, I got this bug sometime ago and lose oops message.
> But, the bug looks trivial and no need to attach oops.
> 
> v4: fix cache size bug s/cache->size/obj_cache->size/
> v3: fix build warning
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  mm/kasan/quarantine.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..b2e1827 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -238,30 +238,23 @@ static void qlist_move_cache(struct qlist_head *from,
>  				   struct qlist_head *to,
>  				   struct kmem_cache *cache)
>  {
> -	struct qlist_node *prev = NULL, *curr;
> +	struct qlist_node *curr;
>  
>  	if (unlikely(qlist_empty(from)))
>  		return;
>  
>  	curr = from->head;
> +	qlist_init(from);
>  	while (curr) {
>  		struct qlist_node *qlink = curr;
>  		struct kmem_cache *obj_cache = qlink_to_cache(qlink);
>  
> -		if (obj_cache == cache) {
> -			if (unlikely(from->head == qlink)) {
> -				from->head = curr->next;
> -				prev = curr;
> -			} else
> -				prev->next = curr->next;
> -			if (unlikely(from->tail == qlink))
> -				from->tail = curr->next;
> -			from->bytes -= cache->size;
> -			qlist_put(to, qlink, cache->size);
> -		} else {
> -			prev = curr;
> -		}
>  		curr = curr->next;

Nit: Wouldn't be more appropriate to swap 'curr' and 'qlink' variable names?
Because now qlink is acts as a "current" pointer.

> +
> +		if (obj_cache == cache)
> +			qlist_put(to, qlink, obj_cache->size);
> +		else
> +			qlist_put(from, qlink, obj_cache->size);
>  	}
>  }
>  
> 

Re: [PATCH v4] kasan/quarantine: fix bugs on qlist_move_cache()

[Dmitry Vyukov]

On Mon, Jul 4, 2016 at 6:31 AM,  <js1304@gmail.com> wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> There are two bugs on qlist_move_cache(). One is that qlist's tail
> isn't set properly. curr->next can be NULL since it is singly linked
> list and NULL value on tail is invalid if there is one item on qlist.
> Another one is that if cache is matched, qlist_put() is called and
> it will set curr->next to NULL. It would cause to stop the loop
> prematurely.
>
> These problems come from complicated implementation so I'd like to
> re-implement it completely. Implementation in this patch is really
> simple. Iterate all qlist_nodes and put them to appropriate list.
>
> Unfortunately, I got this bug sometime ago and lose oops message.
> But, the bug looks trivial and no need to attach oops.
>
> v4: fix cache size bug s/cache->size/obj_cache->size/
> v3: fix build warning
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  mm/kasan/quarantine.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> index 4973505..b2e1827 100644
> --- a/mm/kasan/quarantine.c
> +++ b/mm/kasan/quarantine.c
> @@ -238,30 +238,23 @@ static void qlist_move_cache(struct qlist_head *from,
>                                    struct qlist_head *to,
>                                    struct kmem_cache *cache)
>  {
> -       struct qlist_node *prev = NULL, *curr;
> +       struct qlist_node *curr;
>
>         if (unlikely(qlist_empty(from)))
>                 return;
>
>         curr = from->head;
> +       qlist_init(from);
>         while (curr) {
>                 struct qlist_node *qlink = curr;
>                 struct kmem_cache *obj_cache = qlink_to_cache(qlink);
>
> -               if (obj_cache == cache) {
> -                       if (unlikely(from->head == qlink)) {
> -                               from->head = curr->next;
> -                               prev = curr;
> -                       } else
> -                               prev->next = curr->next;
> -                       if (unlikely(from->tail == qlink))
> -                               from->tail = curr->next;
> -                       from->bytes -= cache->size;
> -                       qlist_put(to, qlink, cache->size);
> -               } else {
> -                       prev = curr;
> -               }
>                 curr = curr->next;
> +
> +               if (obj_cache == cache)
> +                       qlist_put(to, qlink, obj_cache->size);
> +               else
> +                       qlist_put(from, qlink, obj_cache->size);
>         }
>  }

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

Thanks for fixing this!

Re: [PATCH v4] kasan/quarantine: fix bugs on qlist_move_cache()

[Joonsoo Kim]

On Mon, Jul 04, 2016 at 12:49:08PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 07/04/2016 07:31 AM, js1304@gmail.com wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > There are two bugs on qlist_move_cache(). One is that qlist's tail
> > isn't set properly. curr->next can be NULL since it is singly linked
> > list and NULL value on tail is invalid if there is one item on qlist.
> > Another one is that if cache is matched, qlist_put() is called and
> > it will set curr->next to NULL. It would cause to stop the loop
> > prematurely.
> > 
> > These problems come from complicated implementation so I'd like to
> > re-implement it completely. Implementation in this patch is really
> > simple. Iterate all qlist_nodes and put them to appropriate list.
> > 
> > Unfortunately, I got this bug sometime ago and lose oops message.
> > But, the bug looks trivial and no need to attach oops.
> > 
> > v4: fix cache size bug s/cache->size/obj_cache->size/
> > v3: fix build warning
> > 
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > ---
> >  mm/kasan/quarantine.c | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/mm/kasan/quarantine.c b/mm/kasan/quarantine.c
> > index 4973505..b2e1827 100644
> > --- a/mm/kasan/quarantine.c
> > +++ b/mm/kasan/quarantine.c
> > @@ -238,30 +238,23 @@ static void qlist_move_cache(struct qlist_head *from,
> >  				   struct qlist_head *to,
> >  				   struct kmem_cache *cache)
> >  {
> > -	struct qlist_node *prev = NULL, *curr;
> > +	struct qlist_node *curr;
> >  
> >  	if (unlikely(qlist_empty(from)))
> >  		return;
> >  
> >  	curr = from->head;
> > +	qlist_init(from);
> >  	while (curr) {
> >  		struct qlist_node *qlink = curr;
> >  		struct kmem_cache *obj_cache = qlink_to_cache(qlink);
> >  
> > -		if (obj_cache == cache) {
> > -			if (unlikely(from->head == qlink)) {
> > -				from->head = curr->next;
> > -				prev = curr;
> > -			} else
> > -				prev->next = curr->next;
> > -			if (unlikely(from->tail == qlink))
> > -				from->tail = curr->next;
> > -			from->bytes -= cache->size;
> > -			qlist_put(to, qlink, cache->size);
> > -		} else {
> > -			prev = curr;
> > -		}
> >  		curr = curr->next;
> 
> Nit: Wouldn't be more appropriate to swap 'curr' and 'qlink' variable names?
> Because now qlink is acts as a "current" pointer.

Okay. I sent fixed version.

Thanks.