Re: [Patch net 00/16] net_sched: fix races with RCU callbacks

On Mon, Oct 30, 2017 at 6:03 PM, Lucas Bates [off-list ref] wrote:

On Oct 30, 2017 19:13, "Cong Wang" [off-list ref] wrote:

quoted

On Mon, Oct 30, 2017 at 3:39 PM, Lucas Bates [off-list ref] wrote:

quoted

e.On Thu, Oct 26, 2017 at 9:24 PM, Cong Wang [off-list ref]
wrote:

quoted

Recently, the RCU callbacks used in TC filters and TC actions keep
drawing my attention, they introduce at least 4 race condition bugs:

<snip>

quoted

As suggested by Paul, we could defer the work to a workqueue and
gain the permission of holding RTNL again without any performance
impact, however, in tcf_block_put() we could have a deadlock when
flushing workqueue while hodling RTNL lock, the trick here is to
defer the work itself in workqueue and make it queued after all
other works so that we keep the same ordering to avoid any
use-after-free. Please see the first patch for details.

Cong, I don't believe the problem's been resolved just yet....  I have
a new kernel, compiled just today and I'm still tripping over a kernel
bug in this scenario when I run Chris' new test case.

Without a stack trace, I can't do anything. "a kernel bug" could
be anything, why do you believe it is caused by this patchset?

The stack trace I saw touched on the same code that was affected by the
patchset.  I've attached a photo of the trace - sorry, I should have sent it
earlier.  I also apologize for having to send a photo but I was doing the
testing on a small device lacking any kind of serial console access.

Can you try this patch? From your stack trace it is not clear where
the cause is, but we know that the crash is in __tcf_idr_release(),
this is how I came up with the following patch:

diff --git a/include/net/act_api.h b/include/net/act_api.h
index b944e0eb93be..5072446d5f06 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -122,7 +122,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,

 static inline void tc_action_net_exit(struct tc_action_net *tn)
 {
+       rtnl_lock();
        tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
+       rtnl_unlock();
        kfree(tn->idrinfo);
 }

If you need more information or need me to try something else, I'll be able
to tomorrow.

I will look deeper tomorrow. It doesn't look like caused by this patchset
so far, probably yet another missing rtnl like what the above patch shows.


Thanks!

On 17-10-31 01:44 AM, Cong Wang wrote:

On Mon, Oct 30, 2017 at 6:03 PM, Lucas Bates [off-list ref] wrote:

quoted

On Oct 30, 2017 19:13, "Cong Wang" [off-list ref] wrote:

quoted

Looks like an excellent bet;-> tdc kills the container at the end of the
test.

cheers,
jamal

On Tue, Oct 31, 2017 at 7:00 AM, Jamal Hadi Salim [off-list ref] wrote:

On 17-10-31 01:44 AM, Cong Wang wrote:

quoted

Can you try this patch? From your stack trace it is not clear where
the cause is, but we know that the crash is in __tcf_idr_release(),
this is how I came up with the following patch:

diff --git a/include/net/act_api.h b/include/net/act_api.h
index b944e0eb93be..5072446d5f06 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -122,7 +122,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops

*ops,

  static inline void tc_action_net_exit(struct tc_action_net *tn)
  {
+       rtnl_lock();
         tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
+       rtnl_unlock();
         kfree(tn->idrinfo);
  }

Looks like an excellent bet;-> tdc kills the container at the end of the
test.

Unfortunately it doesn't seem to have had any effect, I'm still seeing
the same bug as yesterday. At Jamal's suggestion I put in a delay
after tdc completed running the tests but before it deleted the
container - and I didn't run into the bug after many runs. I had no
luck getting serial console access on any of our other systems so I
have another stack trace photo which I'll send to you directly.

On Tue, Oct 31, 2017 at 2:55 PM, Lucas Bates [off-list ref] wrote:

On Tue, Oct 31, 2017 at 7:00 AM, Jamal Hadi Salim [off-list ref] wrote:

quoted

On 17-10-31 01:44 AM, Cong Wang wrote:

quoted

Can you try this patch? From your stack trace it is not clear where
the cause is, but we know that the crash is in __tcf_idr_release(),
this is how I came up with the following patch:

diff --git a/include/net/act_api.h b/include/net/act_api.h
index b944e0eb93be..5072446d5f06 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -122,7 +122,9 @@ void tcf_idrinfo_destroy(const struct tc_action_ops

*ops,

  static inline void tc_action_net_exit(struct tc_action_net *tn)
  {
+       rtnl_lock();
         tcf_idrinfo_destroy(tn->ops, tn->idrinfo);
+       rtnl_unlock();
         kfree(tn->idrinfo);
  }

Looks like an excellent bet;-> tdc kills the container at the end of the
test.

Unfortunately it doesn't seem to have had any effect, I'm still seeing
the same bug as yesterday. At Jamal's suggestion I put in a delay
after tdc completed running the tests but before it deleted the
container - and I didn't run into the bug after many runs. I had no
luck getting serial console access on any of our other systems so I
have another stack trace photo which I'll send to you directly.

I forgot to mention: it appears the bug appears to be related to
deleting the container. Test d052 is the last to run before tdc does
its teardown and deletes the container; if I add the sleep the bug
never gets triggered.

On Tue, Oct 31, 2017 at 12:13 PM, Lucas Bates [off-list ref] wrote:

On Tue, Oct 31, 2017 at 2:55 PM, Lucas Bates [off-list ref] wrote:

quoted

Unfortunately it doesn't seem to have had any effect, I'm still seeing
the same bug as yesterday. At Jamal's suggestion I put in a delay

As I replied to you privately, it is probably because idrinfo is freed
before action itself. So just RTNL is not enough.

quoted

after tdc completed running the tests but before it deleted the
container - and I didn't run into the bug after many runs. I had no
luck getting serial console access on any of our other systems so I
have another stack trace photo which I'll send to you directly.

I forgot to mention: it appears the bug appears to be related to
deleting the container. Test d052 is the last to run before tdc does
its teardown and deletes the container; if I add the sleep the bug
never gets triggered.

This almost rules out the guilty of this patchset.

I will provide a patch for you to test, since I can't reproduce it here.

Thanks!

On Tue, Oct 31, 2017 at 3:09 PM, Cong Wang [off-list ref] wrote:

This almost rules out the guilty of this patchset.

I will provide a patch for you to test, since I can't reproduce it here.

Lucas, please test the attached patch, it applies to latest -net.

Note, it is a combination of 3 patches which together close the
use-after-free you reported here, I hope.

Please let me know if this works. My basic tests run well without
any stack traces or memory leaks.

Thanks!

On Tue, Oct 31, 2017 at 7:02 PM, Cong Wang [off-list ref] wrote:

On Tue, Oct 31, 2017 at 3:09 PM, Cong Wang [off-list ref] wrote:

quoted

This almost rules out the guilty of this patchset.

I will provide a patch for you to test, since I can't reproduce it here.

Lucas, please test the attached patch, it applies to latest -net.

Note, it is a combination of 3 patches which together close the
use-after-free you reported here, I hope.

Please let me know if this works. My basic tests run well without
any stack traces or memory leaks.

The results look good to me.

I compiled with the patch and ran the test again in a loop. I set it
to run 50 times in a row and didn't trigger the bug once (previous
record was 4 times).

On Wed, Nov 1, 2017 at 9:55 AM, Lucas Bates [off-list ref] wrote:

On Tue, Oct 31, 2017 at 7:02 PM, Cong Wang [off-list ref] wrote:

quoted

On Tue, Oct 31, 2017 at 3:09 PM, Cong Wang [off-list ref] wrote:

quoted

This almost rules out the guilty of this patchset.

I will provide a patch for you to test, since I can't reproduce it here.

Lucas, please test the attached patch, it applies to latest -net.

Note, it is a combination of 3 patches which together close the
use-after-free you reported here, I hope.

Please let me know if this works. My basic tests run well without
any stack traces or memory leaks.

The results look good to me.

I compiled with the patch and ran the test again in a loop. I set it
to run 50 times in a row and didn't trigger the bug once (previous
record was 4 times).

Thanks a lot! I will add your Reported-by and Tested-by and send
out the patches.