LinuxLists.cc - Linux 4.4.34

Attachments:

(No filename) (851.00 B)
signature.asc (181.00 B)
Digital signature Download all attachments

2016-11-22 17:57:53

by Andre Noll

[permalink] [raw]

Subject: Re: Linux 4.4.34

On Tue, Nov 22, 09:14, Eric Dumazet wrote
> We definitely want to fix the real bug, not working around it.
>
> Seems an aliasing problem, key_control and key_basic might point to
> adjacent memory
> and a barrier() would solve the issue as well.

This was also my first idea. I added some printk statements
which confirmed that key_basic->n_proto, key_basic->ip_proto and
key_control->thoff were indeed close to each other. They did not
overlap though.

Hope this helps
Andre
--
Max Planck Institute for Developmental Biology
Spemannstraße 35, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

Attachments:

(No filename) (636.00 B)
signature.asc (181.00 B)
Digital signature Download all attachments

2016-11-22 18:01:31

On Tue, Nov 22, 09:56, Eric Dumazet wrote
> >> @@ -157,6 +157,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> >> memcpy(key_eth_addrs, &eth->h_dest, sizeof(*key_eth_addrs));
> >> }
> >>
> >> + barrier();
> >> again:
> >> switch (proto) {
> >> case htons(ETH_P_IP): {
> >
> > This patch on top of v4.4.34 makes no difference: I'm still getting
> > the panic in eth_type_trans().
> >
>
> What compiler are you using exactly ?

gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3, as shipped with
Ubuntu-12.04. I've also tried gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3)
4.8.4 from Ubuntu-14.04. No difference.

> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 69e4463a4b1b..48791f372aa2 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -551,6 +551,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>
> key_control->thoff = (u16)nhoff;
> out:
> + barrier();
> key_basic->n_proto = proto;
> key_basic->ip_proto = ip_proto;

No luck: Still the same panic.

Andre
--
Max Planck Institute for Developmental Biology
Spemannstraße 35, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

Attachments:

(No filename) (1.22 kB)
signature.asc (181.00 B)
Digital signature Download all attachments

2016-11-22 18:06:30

by Duyck, Alexander H

[permalink] [raw]

Subject: Re: Linux 4.4.34

On Tue, 2016-11-22 at 09:56 -0800, Eric Dumazet wrote:
> On Tue, Nov 22, 2016 at 9:55 AM, Andre Noll <[email protected]> wrote:
> >
> > On Tue, Nov 22, 09:46, Eric Dumazet wrote
> > >
> > > This is an aliasing problem.
> > > Tom code is hard to read and understand.
> > >
> > > Andre, could you try :
> > >
> > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > > index 69e4463a4b1b..b045980faaea 100644
> > > --- a/net/core/flow_dissector.c
> > > +++ b/net/core/flow_dissector.c
> > > @@ -157,6 +157,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> > > memcpy(key_eth_addrs, &eth->h_dest, sizeof(*key_eth_addrs));
> > > }
> > >
> > > + barrier();
> > > again:
> > > switch (proto) {
> > > case htons(ETH_P_IP): {
> >
> > This patch on top of v4.4.34 makes no difference: I'm still getting
> > the panic in eth_type_trans().
> >
>
> What compiler are you using exactly ?
>
> Please try :
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 69e4463a4b1b..48791f372aa2 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -551,6 +551,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>
> key_control->thoff = (u16)nhoff;
> out:
> + barrier();
> key_basic->n_proto = proto;
> key_basic->ip_proto = ip_proto;

Okay so things are starting to make sense for what I was seeing.

I think key_control and key_basic are actually the same pointer. What
has been happening is that storing the network proto is completely
overwriting the network header offset with the value of 8.

Now to just figure out why.

- Alex

2016-11-22 18:09:27

by Duyck, Alexander H

[permalink] [raw]

Subject: Re: Linux 4.4.34

On Tue, 2016-11-22 at 19:06 +0100, Andre Noll wrote:
> On Tue, Nov 22, 09:56, Eric Dumazet wrote
> >
> > >
> > > >
> > > > @@ -157,6 +157,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> > > > memcpy(key_eth_addrs, &eth->h_dest, sizeof(*key_eth_addrs));
> > > > }
> > > >
> > > > + barrier();
> > > > again:
> > > > switch (proto) {
> > > > case htons(ETH_P_IP): {
> > >
> > > This patch on top of v4.4.34 makes no difference: I'm still getting
> > > the panic in eth_type_trans().
> > >
> >
> > What compiler are you using exactly ?
>
> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3, as shipped with
> Ubuntu-12.04. I've also tried gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3)
> 4.8.4 from Ubuntu-14.04. No difference.
>
> >
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 69e4463a4b1b..48791f372aa2 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -551,6 +551,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> >
> > key_control->thoff = (u16)nhoff;
> > out:
> > + barrier();
> > key_basic->n_proto = proto;
> > key_basic->ip_proto = ip_proto;
>
> No luck: Still the same panic.
>
> Andre

Okay I think I have figured it out, but I am not sure what a good
solution is.

I think the problem is the fact that the keys may not be initialized
until init_default_flow_dissectors is called and I am not sure that is
happening before the network interface is trying to do DHCP.

I have to look at the initialization framework to verify if that is the
case. It would make sense why I don't usually see it though since I
usually run igb as a module and that doesn't get loaded until later if
I am not mistaken.

- Alex

2016-11-22 18:11:00

On Tue, Nov 22, 18:08, Duyck, Alexander H wrote
> Okay I think I have figured it out, but I am not sure what a good
> solution is.
>
> I think the problem is the fact that the keys may not be initialized
> until init_default_flow_dissectors is called and I am not sure that is
> happening before the network interface is trying to do DHCP.
>
> I have to look at the initialization framework to verify if that is the
> case. It would make sense why I don't usually see it though since I
> usually run igb as a module and that doesn't get loaded until later if
> I am not mistaken.

Sounds plausible. On our kernels the driver (ixgbe btw) is compiled in.

Andre
--
Max Planck Institute for Developmental Biology
Spemannstraße 35, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

Attachments:

(No filename) (823.00 B)
signature.asc (181.00 B)
Digital signature Download all attachments

2016-11-22 19:17:34

by Eric Dumazet

[permalink] [raw]

Subject: [PATCH net] flow_dissect: call init_default_flow_dissectors() earlier

From: Eric Dumazet <[email protected]>

Andre Noll reported panics after my recent fix (commit 34fad54c2537
"net: __skb_flow_dissect() must cap its return value")

After some more headaches, Alexander root caused the problem to
init_default_flow_dissectors() being called too late, in case
a network driver like IGB is not a module and receives DHCP message
very early.

Fix is to call init_default_flow_dissectors() much earlier,
as it is a core infrastructure and does not depend on another
kernel service.

Fixes: 06635a35d13d4 ("flow_dissect: use programable dissector in skb_flow_dissect and friends")
Signed-off-by: Eric Dumazet <[email protected]>
Reported-by: Andre Noll <[email protected]>
Diagnosed-by: Alexander Duyck <[email protected]>
---
net/core/flow_dissector.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 69e4463a4b1b..c6d8207ffa7e 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1013,4 +1013,4 @@ static int __init init_default_flow_dissectors(void)
return 0;
}

-late_initcall_sync(init_default_flow_dissectors);
+core_initcall(init_default_flow_dissectors);

2016-11-22 19:44:22

by Andre Noll

[permalink] [raw]

Subject: Re: [PATCH net] flow_dissect: call init_default_flow_dissectors() earlier

On Tue, Nov 22, 11:17, Eric Dumazet wrote
> -late_initcall_sync(init_default_flow_dissectors);
> +core_initcall(init_default_flow_dissectors);

Indeed, that fixed it. Feel free to add

Tested-by: Andre Noll <[email protected]>

Thanks a lot
Andre
--
Max Planck Institute for Developmental Biology
Spemannstraße 35, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

Attachments:

(No filename) (410.00 B)
signature.asc (181.00 B)
Digital signature Download all attachments

2016-11-22 19:44:51

by David Miller

[permalink] [raw]

Subject: Re: [PATCH net] flow_dissect: call init_default_flow_dissectors() earlier

From: Eric Dumazet <[email protected]>
Date: Tue, 22 Nov 2016 11:17:30 -0800

> From: Eric Dumazet <[email protected]>
>
> Andre Noll reported panics after my recent fix (commit 34fad54c2537
> "net: __skb_flow_dissect() must cap its return value")
>
> After some more headaches, Alexander root caused the problem to
> init_default_flow_dissectors() being called too late, in case
> a network driver like IGB is not a module and receives DHCP message
> very early.
>
> Fix is to call init_default_flow_dissectors() much earlier,
> as it is a core infrastructure and does not depend on another
> kernel service.
>
> Fixes: 06635a35d13d4 ("flow_dissect: use programable dissector in skb_flow_dissect and friends")
> Signed-off-by: Eric Dumazet <[email protected]>
> Reported-by: Andre Noll <[email protected]>
> Diagnosed-by: Alexander Duyck <[email protected]>

Applied and queued up for -stable, I'll try to fast-track this.