Re: Fwd: Re: Kernel panic with 4.16-rc1 (and 4.16-rc2) running selftest

On Fri, Feb 23, 2018 Randy Dunlap wrote:

[add Matthew Wilcox; hopefully he can look/see]

Thanks, Randy.  I don't understand why nobody else thought to cc the
author of the patch that it was bisected to ...

On 02/23/2018 04:13 PM, Cong Wang wrote:

quoted

On Fri, Feb 23, 2018 at 3:27 PM, Cong Wang [off-list ref]

wrote:

quoted

quoted

On Fri, Feb 23, 2018 at 11:00 AM, Randy Dunlap [off-list ref]

wrote:

quoted

quoted

quoted

On 02/23/2018 08:05 AM, Khalid Aziz wrote:

quoted

Same selftest does not cause panic on 4.15. git bisect pointed to

commit 6ce711f2750031d12cec91384ac5cfa0a485b60a ("idr: Make 1-based IDRs
more efficient").

quoted

quoted

quoted

quoted

Kernel config is attached.

Looks like something horribly wrong with u32 key id idr...

Adding a few printk's, I got:

[   31.231560] requested handle = ffe00000
[   31.232426] allocated handle = 0
...
[   31.246475] requested handle = ffd00000
[   31.247555] allocated handle = 1


So the bug is here where we can't allocate a specific handle:

                        err = idr_alloc_u32(&tp_c->handle_idr, ht,

&handle,

quoted

                                            handle, GFP_KERNEL);
                        if (err) {
                                kfree(ht);
                                return err;
                        }

Please try this patch.  It fixes ffe00000, but there may be more things
tested that it may not work for.

Chris Mi, what happened to that set of testcases you promised to write
for me?

diff --git a/lib/idr.c b/lib/idr.c
index c98d77fcf393..10d9b8d47c33 100644
--- a/lib/idr.c
+++ b/lib/idr.c

@@ -36,8 +36,8 @@ int idr_alloc_u32(struct idr *idr, void *ptr, u32 *nextid,
 {
 	struct radix_tree_iter iter;
 	void __rcu **slot;
-	int base = idr->idr_base;
-	int id = *nextid;
+	unsigned int base = idr->idr_base;
+	unsigned int id = *nextid;
 
 	if (WARN_ON_ONCE(radix_tree_is_internal_node(ptr)))
 		return -EINVAL;

-----Original Message-----
From: Matthew Wilcox [mailto:willy@infradead.org]
Sent: Saturday, February 24, 2018 9:15 AM
To: Cong Wang <redacted>; Khalid Aziz
[off-list ref]; linux-kernel@vger.kernel.org;
netdev@vger.kernel.org
Cc: Chris Mi <redacted>
Subject: Re: Fwd: Re: Kernel panic with 4.16-rc1 (and 4.16-rc2) running
selftest

On Fri, Feb 23, 2018 Randy Dunlap wrote:

quoted

[add Matthew Wilcox; hopefully he can look/see]

Thanks, Randy.  I don't understand why nobody else thought to cc the author
of the patch that it was bisected to ...

quoted

On 02/23/2018 04:13 PM, Cong Wang wrote:

quoted

On Fri, Feb 23, 2018 at 3:27 PM, Cong Wang
[off-list ref]

wrote:

quoted

quoted

On Fri, Feb 23, 2018 at 11:00 AM, Randy Dunlap
[off-list ref]

wrote:

quoted

quoted

quoted

On 02/23/2018 08:05 AM, Khalid Aziz wrote:

quoted

Same selftest does not cause panic on 4.15. git bisect pointed to

commit 6ce711f2750031d12cec91384ac5cfa0a485b60a ("idr: Make 1-based
IDRs more efficient").

quoted

quoted

quoted

quoted

Kernel config is attached.

Looks like something horribly wrong with u32 key id idr...

Adding a few printk's, I got:

[   31.231560] requested handle = ffe00000
[   31.232426] allocated handle = 0
...
[   31.246475] requested handle = ffd00000
[   31.247555] allocated handle = 1


So the bug is here where we can't allocate a specific handle:

                        err = idr_alloc_u32(&tp_c->handle_idr, ht,

&handle,

quoted

                                            handle, GFP_KERNEL);
                        if (err) {
                                kfree(ht);
                                return err;
                        }

Please try this patch.  It fixes ffe00000, but there may be more things tested
that it may not work for.

Chris Mi, what happened to that set of testcases you promised to write for
me?

I promised to write it after the API is stabilized since you were going to change it.
I will inform the management about this new task and get back to you later.

To verify this patch, the following is a sanity test case:

# tc qdisc delete dev $link ingress > /dev/null 2>&1;
# tc qdisc add dev $link ingress;
# tc filter add dev $link prio 1 protocol ip handle 0x80000001 parent ffff: flower skip_hw src_mac e4:11:0:0:0:2 dst_mac e4:12:0:0:0:2 action drop;
# tc filter show dev $link parent ffff:

filter pref 1 flower chain 0
filter pref 1 flower chain 0 handle 0x80000001
  dst_mac e4:12:00:00:00:02
  src_mac e4:11:00:00:00:02
  eth_type ipv4
  skip_hw
  not_in_hw
        action order 1: gact action drop
         random type none pass val 0
         index 1 ref 1 bind 1

Please make sure the handle is the same as the user specifies.

On Sat, Feb 24, 2018 at 01:49:35AM +0000, Chris Mi wrote:

To verify this patch, the following is a sanity test case:

# tc qdisc delete dev $link ingress > /dev/null 2>&1;
# tc qdisc add dev $link ingress;
# tc filter add dev $link prio 1 protocol ip handle 0x80000001 parent ffff: flower skip_hw src_mac e4:11:0:0:0:2 dst_mac e4:12:0:0:0:2 action drop;
# tc filter show dev $link parent ffff:

filter pref 1 flower chain 0
filter pref 1 flower chain 0 handle 0x80000001

I added these tests to my local tree for now.

diff --git a/tools/testing/radix-tree/idr-test.c b/tools/testing/radix-tree/idr-test.c
index 44ef9eba5a7a..28d99325a32d 100644
--- a/tools/testing/radix-tree/idr-test.c
+++ b/tools/testing/radix-tree/idr-test.c

@@ -178,6 +178,29 @@ void idr_get_next_test(int base)
 	idr_destroy(&idr);
 }
 
+void idr_u32_test(struct idr *idr, int base)
+{
+	assert(idr_is_empty(idr));
+	idr_init_base(idr, base);
+	u32 handle = 10;
+	idr_alloc_u32(idr, NULL, &handle, handle, GFP_KERNEL);
+	BUG_ON(handle != 10);
+	idr_remove(idr, handle);
+	assert(idr_is_empty(idr));
+
+	handle = 0x80000001;
+	idr_alloc_u32(idr, NULL, &handle, handle, GFP_KERNEL);
+	BUG_ON(handle != 0x80000001);
+	idr_remove(idr, handle);
+	assert(idr_is_empty(idr));
+
+	handle = 0xffe00000;
+	idr_alloc_u32(idr, NULL, &handle, handle, GFP_KERNEL);
+	BUG_ON(handle != 0xffe00000);
+	idr_remove(idr, handle);
+	assert(idr_is_empty(idr));
+}
+
 void idr_checks(void)
 {
 	unsigned long i;

@@ -248,6 +271,9 @@ void idr_checks(void)
 	idr_get_next_test(0);
 	idr_get_next_test(1);
 	idr_get_next_test(4);
+	idr_u32_test(&idr, 0);
+	idr_u32_test(&idr, 1);
+	idr_u32_test(&idr, 4);
 }
 
 /*

Hi Matthew,

Sorry for the late response. I'll add the idr test cases for the new 
APIs ASAP.

Thanks,
Chris

On 2/24/2018 10:46 AM, Matthew Wilcox wrote:

On 02/23/2018 06:15 PM, Matthew Wilcox wrote:

On Fri, Feb 23, 2018 Randy Dunlap wrote:

quoted

[add Matthew Wilcox; hopefully he can look/see]

Thanks, Randy.  I don't understand why nobody else thought to cc the
author of the patch that it was bisected to ...

Sorry, Willy. That was my fault. I should have cc'd you to begin with.

Please try this patch.  It fixes ffe00000, but there may be more things
tested that it may not work for.

This patch fixes the problem. I do not see kernel panics with this patch 
any more.

--
Khalid