From: Zqiang <[email protected]>
Add kasan_record_aux_stack function for kvfree_call_rcu function to
record call stacks.
Signed-off-by: Zqiang <[email protected]>
---
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
On Wed, Nov 18, 2020 at 11:53:09AM +0800, [email protected] wrote:
> From: Zqiang <[email protected]>
>
> Add kasan_record_aux_stack function for kvfree_call_rcu function to
> record call stacks.
>
> Signed-off-by: Zqiang <[email protected]>
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
>
On Thu, Nov 19, 2020 at 10:49 PM Paul E. McKenney <[email protected]> wrote:
>
> On Wed, Nov 18, 2020 at 11:53:09AM +0800, [email protected] wrote:
> > From: Zqiang <[email protected]>
> >
> > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > record call stacks.
> >
> > Signed-off-by: Zqiang <[email protected]>
>
> 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 <[email protected]>
On Thu, Nov 19, 2020 at 01:49:34PM -0800, Paul E. McKenney wrote:
> On Wed, Nov 18, 2020 at 11:53:09AM +0800, [email protected] wrote:
> > From: Zqiang <[email protected]>
> >
> > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > record call stacks.
> >
> > Signed-off-by: Zqiang <[email protected]>
>
> 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
On Fri, Nov 20, 2020 at 12:59 PM Uladzislau Rezki <[email protected]> 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, [email protected] wrote:
> > > From: Zqiang <[email protected]>
> > >
> > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > record call stacks.
> > >
> > > Signed-off-by: Zqiang <[email protected]>
> >
> > 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
> On Fri, Nov 20, 2020 at 12:59 PM Uladzislau Rezki <[email protected]> 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, [email protected] wrote:
> > > > From: Zqiang <[email protected]>
> > > >
> > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > record call stacks.
> > > >
> > > > Signed-off-by: Zqiang <[email protected]>
> > >
> > > 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) <[email protected]>
Thanks.
--
Vlad Rezki
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 <[email protected]> wrote:
> >
> > On Wed, Nov 18, 2020 at 11:53:09AM +0800, [email protected] wrote:
> > > From: Zqiang <[email protected]>
> > >
> > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > record call stacks.
> > >
> > > Signed-off-by: Zqiang <[email protected]>
> >
> > 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 <[email protected]>
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 <[email protected]> wrote:
> > >
> > > On Wed, Nov 18, 2020 at 11:53:09AM +0800, [email protected] wrote:
> > > > From: Zqiang <[email protected]>
> > > >
> > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > record call stacks.
> > > >
> > > > Signed-off-by: Zqiang <[email protected]>
> > >
> > > 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 <[email protected]>
On Fri, Nov 20, 2020 at 3:34 PM Paul E. McKenney <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Wed, Nov 18, 2020 at 11:53:09AM +0800, [email protected] wrote:
> > > > From: Zqiang <[email protected]>
> > > >
> > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > record call stacks.
> > > >
> > > > Signed-off-by: Zqiang <[email protected]>
> > >
> > > 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.
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 <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Wed, Nov 18, 2020 at 11:53:09AM +0800, [email protected] wrote:
> > > > > From: Zqiang <[email protected]>
> > > > >
> > > > > Add kasan_record_aux_stack function for kvfree_call_rcu function to
> > > > > record call stacks.
> > > > >
> > > > > Signed-off-by: Zqiang <[email protected]>
> > > >
> > > > 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 <[email protected]>
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 <[email protected]>
Reviewed-by: Uladzislau Rezki (Sony) <[email protected]>
Signed-off-by: Zqiang <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
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);