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

[Khalid Aziz]

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

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 ...


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



Please try this patch.  It fixes ffe0, 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


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;

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

[Khalid Aziz]

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

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 ...


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



Please try this patch.  It fixes ffe0, 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


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;

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

[Chris Mi]

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 Sat, Feb 24, 2018 at 01:49:35AM +, 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 0x8001 parent : 
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 :

filter pref 1 flower chain 0
filter pref 1 flower chain 0 handle 0x8001

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();
  }
  
+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, GFP_KERNEL);
+   BUG_ON(handle != 10);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0x8001;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0x8001);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0xffe0;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0xffe0);
+   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(, 0);
+   idr_u32_test(, 1);
+   idr_u32_test(, 4);
  }
  
  /*

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

[Chris Mi]

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 Sat, Feb 24, 2018 at 01:49:35AM +, 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 0x8001 parent : 
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 :

filter pref 1 flower chain 0
filter pref 1 flower chain 0 handle 0x8001

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();
  }
  
+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, GFP_KERNEL);
+   BUG_ON(handle != 10);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0x8001;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0x8001);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0xffe0;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0xffe0);
+   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(, 0);
+   idr_u32_test(, 1);
+   idr_u32_test(, 4);
  }
  
  /*

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

[Matthew Wilcox]

On Sat, Feb 24, 2018 at 01:49:35AM +, 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 0x8001 parent : 
> 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 :
> 
> filter pref 1 flower chain 0
> filter pref 1 flower chain 0 handle 0x8001

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();
 }
 
+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, GFP_KERNEL);
+   BUG_ON(handle != 10);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0x8001;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0x8001);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0xffe0;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0xffe0);
+   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(, 0);
+   idr_u32_test(, 1);
+   idr_u32_test(, 4);
 }
 
 /*

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

[Matthew Wilcox]

On Sat, Feb 24, 2018 at 01:49:35AM +, 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 0x8001 parent : 
> 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 :
> 
> filter pref 1 flower chain 0
> filter pref 1 flower chain 0 handle 0x8001

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();
 }
 
+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, GFP_KERNEL);
+   BUG_ON(handle != 10);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0x8001;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0x8001);
+   idr_remove(idr, handle);
+   assert(idr_is_empty(idr));
+
+   handle = 0xffe0;
+   idr_alloc_u32(idr, NULL, , handle, GFP_KERNEL);
+   BUG_ON(handle != 0xffe0);
+   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(, 0);
+   idr_u32_test(, 1);
+   idr_u32_test(, 4);
 }
 
 /*

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

[Chris Mi]

> -Original Message-
> From: Matthew Wilcox [mailto:wi...@infradead.org]
> Sent: Saturday, February 24, 2018 9:15 AM
> To: Cong Wang <xiyou.wangc...@gmail.com>; Khalid Aziz
> <khalid.a...@oracle.com>; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org
> Cc: Chris Mi <chr...@mellanox.com>
> Subject: 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:
> > > On Fri, Feb 23, 2018 at 3:27 PM, Cong Wang
> > > <xiyou.wangc...@gmail.com>
> > wrote:
> > >> On Fri, Feb 23, 2018 at 11:00 AM, Randy Dunlap
> > >> <rdun...@infradead.org>
> > wrote:
> > >>> On 02/23/2018 08:05 AM, Khalid Aziz wrote:
> > >>>> Same selftest does not cause panic on 4.15. git bisect pointed to
> > commit 6ce711f2750031d12cec91384ac5cfa0a485b60a ("idr: Make 1-based
> > IDRs more efficient").
> > >>>> 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 = ffe0
> > > [   31.232426] allocated handle = 0
> > > ...
> > > [   31.246475] requested handle = ffd0
> > > [   31.247555] allocated handle = 1
> > >
> > >
> > > So the bug is here where we can't allocate a specific handle:
> > >
> > > err = idr_alloc_u32(_c->handle_idr, ht,
> > ,
> > > handle, GFP_KERNEL);
> > > if (err) {
> > > kfree(ht);
> > > return err;
> > > }
> 
> Please try this patch.  It fixes ffe0, 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.
> 
> 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;
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 0x8001 parent : 
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 :

filter pref 1 flower chain 0
filter pref 1 flower chain 0 handle 0x8001
  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.

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

[Chris Mi]

> -Original Message-
> From: Matthew Wilcox [mailto:wi...@infradead.org]
> Sent: Saturday, February 24, 2018 9:15 AM
> To: Cong Wang ; Khalid Aziz
> ; linux-kernel@vger.kernel.org;
> net...@vger.kernel.org
> Cc: Chris Mi 
> Subject: 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:
> > > On Fri, Feb 23, 2018 at 3:27 PM, Cong Wang
> > > 
> > wrote:
> > >> On Fri, Feb 23, 2018 at 11:00 AM, Randy Dunlap
> > >> 
> > wrote:
> > >>> On 02/23/2018 08:05 AM, Khalid Aziz wrote:
> > >>>> Same selftest does not cause panic on 4.15. git bisect pointed to
> > commit 6ce711f2750031d12cec91384ac5cfa0a485b60a ("idr: Make 1-based
> > IDRs more efficient").
> > >>>> 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 = ffe0
> > > [   31.232426] allocated handle = 0
> > > ...
> > > [   31.246475] requested handle = ffd0
> > > [   31.247555] allocated handle = 1
> > >
> > >
> > > So the bug is here where we can't allocate a specific handle:
> > >
> > > err = idr_alloc_u32(_c->handle_idr, ht,
> > ,
> > > handle, GFP_KERNEL);
> > > if (err) {
> > > kfree(ht);
> > > return err;
> > > }
> 
> Please try this patch.  It fixes ffe0, 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.
> 
> 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;
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 0x8001 parent : 
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 :

filter pref 1 flower chain 0
filter pref 1 flower chain 0 handle 0x8001
  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.

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

[Matthew Wilcox]

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:
> > On Fri, Feb 23, 2018 at 3:27 PM, Cong Wang 
> wrote:
> >> On Fri, Feb 23, 2018 at 11:00 AM, Randy Dunlap 
> wrote:
> >>> On 02/23/2018 08:05 AM, Khalid Aziz wrote:
>  Same selftest does not cause panic on 4.15. git bisect pointed to
> commit 6ce711f2750031d12cec91384ac5cfa0a485b60a ("idr: Make 1-based IDRs
> more efficient").
>  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 = ffe0
> > [   31.232426] allocated handle = 0
> > ...
> > [   31.246475] requested handle = ffd0
> > [   31.247555] allocated handle = 1
> >
> >
> > So the bug is here where we can't allocate a specific handle:
> >
> > err = idr_alloc_u32(_c->handle_idr, ht,
> ,
> > handle, GFP_KERNEL);
> > if (err) {
> > kfree(ht);
> > return err;
> > }

Please try this patch.  It fixes ffe0, 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;

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

[Matthew Wilcox]

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:
> > On Fri, Feb 23, 2018 at 3:27 PM, Cong Wang 
> wrote:
> >> On Fri, Feb 23, 2018 at 11:00 AM, Randy Dunlap 
> wrote:
> >>> On 02/23/2018 08:05 AM, Khalid Aziz wrote:
>  Same selftest does not cause panic on 4.15. git bisect pointed to
> commit 6ce711f2750031d12cec91384ac5cfa0a485b60a ("idr: Make 1-based IDRs
> more efficient").
>  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 = ffe0
> > [   31.232426] allocated handle = 0
> > ...
> > [   31.246475] requested handle = ffd0
> > [   31.247555] allocated handle = 1
> >
> >
> > So the bug is here where we can't allocate a specific handle:
> >
> > err = idr_alloc_u32(_c->handle_idr, ht,
> ,
> > handle, GFP_KERNEL);
> > if (err) {
> > kfree(ht);
> > return err;
> > }

Please try this patch.  It fixes ffe0, 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;