[PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[qiang.zhang]

From: Zqiang <qiang.zhang@windriver.com>

Add kasan_record_aux_stack function for kvfree_call_rcu function to
record call stacks.

Signed-off-by: Zqiang <qiang.zhang@windriver.com>
---
 kernel/rcu/tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index da3414522285..a252b2f0208d 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 		success = true;
 		goto unlock_return;
 	}
-
+	kasan_record_aux_stack(ptr);
 	success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
 	if (!success) {
 		run_page_cache_worker(krcp);
-- 
2.17.1

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Paul E. McKenney]

On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> From: Zqiang <qiang.zhang@windriver.com>
> 
> Add kasan_record_aux_stack function for kvfree_call_rcu function to
> record call stacks.
> 
> Signed-off-by: Zqiang <qiang.zhang@windriver.com>

Thank you, but this does not apply on the "dev" branch of the -rcu tree.
See file:///home/git/kernel.org/rcutodo.html for more info.

Adding others on CC who might have feedback on the general approach.

							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index da3414522285..a252b2f0208d 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  		success = true;
>  		goto unlock_return;
>  	}
> -
> +	kasan_record_aux_stack(ptr);
>  	success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
>  	if (!success) {
>  		run_page_cache_worker(krcp);
> -- 
> 2.17.1
> 

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Dmitry Vyukov]

On Thu, Nov 19, 2020 at 10:49 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > From: Zqiang <qiang.zhang@windriver.com>
> >
> > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > record call stacks.
> >
> > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
>
> Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> See file:///home/git/kernel.org/rcutodo.html for more info.
>
> Adding others on CC who might have feedback on the general approach.
>
>                                                         Thanx, Paul
>
> > ---
> >  kernel/rcu/tree.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index da3414522285..a252b2f0208d 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> >               success = true;
> >               goto unlock_return;
> >       }
> > -
> > +     kasan_record_aux_stack(ptr);
> >       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
> >       if (!success) {
> >               run_page_cache_worker(krcp);


kvfree_call_rcu is intended to free objects, right? If so this is:

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

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Uladzislau Rezki]

On Thu, Nov 19, 2020 at 01:49:34PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > From: Zqiang <qiang.zhang@windriver.com>
> > 
> > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > record call stacks.
> > 
> > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> 
> Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> See file:///home/git/kernel.org/rcutodo.html for more info.
> 
> Adding others on CC who might have feedback on the general approach.
> 
> 							Thanx, Paul
> 
> > ---
> >  kernel/rcu/tree.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index da3414522285..a252b2f0208d 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> >  		success = true;
> >  		goto unlock_return;
> >  	}
> > -
> > +	kasan_record_aux_stack(ptr);
Is that save to invoke it on vmalloced ptr.?

--
Vlad Rezki

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Dmitry Vyukov]

On Fri, Nov 20, 2020 at 12:59 PM Uladzislau Rezki <urezki@gmail.com> wrote:
>
> On Thu, Nov 19, 2020 at 01:49:34PM -0800, Paul E. McKenney wrote:
> > On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > > From: Zqiang <qiang.zhang@windriver.com>
> > >
> > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > record call stacks.
> > >
> > > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> >
> > Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> > See file:///home/git/kernel.org/rcutodo.html for more info.
> >
> > Adding others on CC who might have feedback on the general approach.
> >
> >                                                       Thanx, Paul
> >
> > > ---
> > >  kernel/rcu/tree.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index da3414522285..a252b2f0208d 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > >             success = true;
> > >             goto unlock_return;
> > >     }
> > > -
> > > +   kasan_record_aux_stack(ptr);
> Is that save to invoke it on vmalloced ptr.?

Yes, kasan_record_aux_stack should figure it out itself.
We call kasan_record_aux_stack on call_rcu as well, and rcu structs
can be anywhere.
See:
https://elixir.bootlin.com/linux/v5.10-rc4/source/mm/kasan/generic.c#L335

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Uladzislau Rezki]

> On Fri, Nov 20, 2020 at 12:59 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> >
> > On Thu, Nov 19, 2020 at 01:49:34PM -0800, Paul E. McKenney wrote:
> > > On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > > > From: Zqiang <qiang.zhang@windriver.com>
> > > >
> > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > record call stacks.
> > > >
> > > > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> > >
> > > Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> > > See file:///home/git/kernel.org/rcutodo.html for more info.
> > >
> > > Adding others on CC who might have feedback on the general approach.
> > >
> > >                                                       Thanx, Paul
> > >
> > > > ---
> > > >  kernel/rcu/tree.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index da3414522285..a252b2f0208d 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > >             success = true;
> > > >             goto unlock_return;
> > > >     }
> > > > -
> > > > +   kasan_record_aux_stack(ptr);
> > Is that save to invoke it on vmalloced ptr.?
> 
> Yes, kasan_record_aux_stack should figure it out itself.
> We call kasan_record_aux_stack on call_rcu as well, and rcu structs
> can be anywhere.
> See:
> https://elixir.bootlin.com/linux/v5.10-rc4/source/mm/kasan/generic.c#L335
>
Ah, i see the check. Just a minor comment, please also add a
clear comment about intention of doing such record.

Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Thanks.

--
Vlad Rezki

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Paul E. McKenney]

On Fri, Nov 20, 2020 at 09:51:15AM +0100, Dmitry Vyukov wrote:
> On Thu, Nov 19, 2020 at 10:49 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > > From: Zqiang <qiang.zhang@windriver.com>
> > >
> > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > record call stacks.
> > >
> > > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> >
> > Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> > See file:///home/git/kernel.org/rcutodo.html for more info.
> >
> > Adding others on CC who might have feedback on the general approach.
> >
> >                                                         Thanx, Paul
> >
> > > ---
> > >  kernel/rcu/tree.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index da3414522285..a252b2f0208d 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > >               success = true;
> > >               goto unlock_return;
> > >       }
> > > -
> > > +     kasan_record_aux_stack(ptr);
> > >       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
> > >       if (!success) {
> > >               run_page_cache_worker(krcp);
> 
> kvfree_call_rcu is intended to free objects, right? If so this is:

True, but mightn't there still be RCU readers referencing this object for
some time, as in up to the point that the RCU grace period ends?  If so,
won't adding this cause KASAN to incorrectly complain about those readers?

Or am I missing something here?

						Thanx, Paul

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

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Dmitry Vyukov]

On Fri, Nov 20, 2020 at 3:34 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Fri, Nov 20, 2020 at 09:51:15AM +0100, Dmitry Vyukov wrote:
> > On Thu, Nov 19, 2020 at 10:49 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> > >
> > > On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > > > From: Zqiang <qiang.zhang@windriver.com>
> > > >
> > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > record call stacks.
> > > >
> > > > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> > >
> > > Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> > > See file:///home/git/kernel.org/rcutodo.html for more info.
> > >
> > > Adding others on CC who might have feedback on the general approach.
> > >
> > >                                                         Thanx, Paul
> > >
> > > > ---
> > > >  kernel/rcu/tree.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index da3414522285..a252b2f0208d 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > >               success = true;
> > > >               goto unlock_return;
> > > >       }
> > > > -
> > > > +     kasan_record_aux_stack(ptr);
> > > >       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
> > > >       if (!success) {
> > > >               run_page_cache_worker(krcp);
> >
> > kvfree_call_rcu is intended to free objects, right? If so this is:
>
> True, but mightn't there still be RCU readers referencing this object for
> some time, as in up to the point that the RCU grace period ends?  If so,
> won't adding this cause KASAN to incorrectly complain about those readers?
>
> Or am I missing something here?

kvfree_call_rcu does not check anything, not poison the object for
future accesses (it is also called in call_rcu which does not
necessarily free the object).
It just notes the current stack to provide in reports later.
The problem is that the free stack is pointless for objects freed by
rcu. In such cases we want call_rcu/kvfree_call_rcu stack in
use-after-free reports.

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Paul E. McKenney]

On Fri, Nov 20, 2020 at 06:34:40AM -0800, Paul E. McKenney wrote:
> On Fri, Nov 20, 2020 at 09:51:15AM +0100, Dmitry Vyukov wrote:
> > On Thu, Nov 19, 2020 at 10:49 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> > >
> > > On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > > > From: Zqiang <qiang.zhang@windriver.com>
> > > >
> > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > record call stacks.
> > > >
> > > > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> > >
> > > Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> > > See file:///home/git/kernel.org/rcutodo.html for more info.
> > >
> > > Adding others on CC who might have feedback on the general approach.
> > >
> > >                                                         Thanx, Paul
> > >
> > > > ---
> > > >  kernel/rcu/tree.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index da3414522285..a252b2f0208d 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > >               success = true;
> > > >               goto unlock_return;
> > > >       }
> > > > -
> > > > +     kasan_record_aux_stack(ptr);
> > > >       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
> > > >       if (!success) {
> > > >               run_page_cache_worker(krcp);
> > 
> > kvfree_call_rcu is intended to free objects, right? If so this is:
> 
> True, but mightn't there still be RCU readers referencing this object for
> some time, as in up to the point that the RCU grace period ends?  If so,
> won't adding this cause KASAN to incorrectly complain about those readers?
> 
> Or am I missing something here?

For example, is kasan_record_aux_stack() -only- recording the stack and
not at all updating the allocated/freed state?

						Thanx, Paul

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

Re: [PATCH] rcu: kasan: record and print kvfree_call_rcu call stack

[Paul E. McKenney]

On Fri, Nov 20, 2020 at 03:44:04PM +0100, Dmitry Vyukov wrote:
> On Fri, Nov 20, 2020 at 3:34 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> >
> > On Fri, Nov 20, 2020 at 09:51:15AM +0100, Dmitry Vyukov wrote:
> > > On Thu, Nov 19, 2020 at 10:49 PM Paul E. McKenney <paulmck@kernel.org> wrote:
> > > >
> > > > On Wed, Nov 18, 2020 at 11:53:09AM +0800, qiang.zhang@windriver.com wrote:
> > > > > From: Zqiang <qiang.zhang@windriver.com>
> > > > >
> > > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > > record call stacks.
> > > > >
> > > > > Signed-off-by: Zqiang <qiang.zhang@windriver.com>
> > > >
> > > > Thank you, but this does not apply on the "dev" branch of the -rcu tree.
> > > > See file:///home/git/kernel.org/rcutodo.html for more info.
> > > >
> > > > Adding others on CC who might have feedback on the general approach.
> > > >
> > > >                                                         Thanx, Paul
> > > >
> > > > > ---
> > > > >  kernel/rcu/tree.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index da3414522285..a252b2f0208d 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -3506,7 +3506,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> > > > >               success = true;
> > > > >               goto unlock_return;
> > > > >       }
> > > > > -
> > > > > +     kasan_record_aux_stack(ptr);
> > > > >       success = kvfree_call_rcu_add_ptr_to_bulk(krcp, ptr);
> > > > >       if (!success) {
> > > > >               run_page_cache_worker(krcp);
> > >
> > > kvfree_call_rcu is intended to free objects, right? If so this is:
> >
> > True, but mightn't there still be RCU readers referencing this object for
> > some time, as in up to the point that the RCU grace period ends?  If so,
> > won't adding this cause KASAN to incorrectly complain about those readers?
> >
> > Or am I missing something here?
> 
> kvfree_call_rcu does not check anything, not poison the object for
> future accesses (it is also called in call_rcu which does not
> necessarily free the object).
> It just notes the current stack to provide in reports later.
> The problem is that the free stack is pointless for objects freed by
> rcu. In such cases we want call_rcu/kvfree_call_rcu stack in
> use-after-free reports.

OK, sounds good, thank you!

I will take this patch with your ack and Uladzislau's reviewed-by.
I had to forward-port this to -rcu brach "dev", and along the way I
updated the commit log to make Dmitry's point above, so please let me
know if I messed anything up.

							Thanx, Paul

------------------------------------------------------------------------

commit 3ce23b2df528877623ffc9c9cc2b6885eb3ae9db
Author: Zqiang <qiang.zhang@windriver.com>
Date:   Fri Nov 20 06:53:11 2020 -0800

    rcu: Record kvfree_call_rcu() call stack for KASAN
    
    This commit adds a call to kasan_record_aux_stack() in kvfree_call_rcu()
    in order to record the call stack of the code that caused the object
    to be freed.  Please note that this function does not update the
    allocated/freed state, which is important because RCU readers might
    still be referencing this object.
    
    Acked-by: Dmitry Vyukov <dvyukov@google.com>
    Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
    Signed-off-by: Zqiang <qiang.zhang@windriver.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1d956f9..4aa7745 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3514,6 +3514,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 		return;
 	}
 
+	kasan_record_aux_stack(ptr);
 	success = add_ptr_to_bulk_krc_lock(&krcp, &flags, ptr, !head);
 	if (!success) {
 		run_page_cache_worker(krcp);