Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1526482 > unrolled thread

Linux 4.4.34

Back to article view | Back to linux.kernel

Contents

  Linux 4.4.34 Greg KH <gregkh@linuxfoundation.org> - 2016-11-21 10:30 +0100
    Re: Linux 4.4.34 Greg KH <gregkh@linuxfoundation.org> - 2016-11-22 18:20 +0100
      Re: Linux 4.4.34 Eric Dumazet <edumazet@google.com> - 2016-11-22 18:20 +0100
        Re: Linux 4.4.34 "Duyck, Alexander H" <alexander.h.duyck@intel.com> - 2016-11-22 18:50 +0100
          Re: Linux 4.4.34 Eric Dumazet <edumazet@google.com> - 2016-11-22 18:50 +0100
            Re: Linux 4.4.34 Andre Noll <maan@tuebingen.mpg.de> - 2016-11-22 19:00 +0100
              Re: Linux 4.4.34 Andre Noll <maan@tuebingen.mpg.de> - 2016-11-22 19:10 +0100
                Re: Linux 4.4.34 "Duyck, Alexander H" <alexander.h.duyck@intel.com> - 2016-11-22 19:10 +0100
                  Re: Linux 4.4.34 Eric Dumazet <edumazet@google.com> - 2016-11-22 19:20 +0100
                  Re: Linux 4.4.34 Andre Noll <maan@tuebingen.mpg.de> - 2016-11-22 19:30 +0100
                    [PATCH net] flow_dissect: call init_default_flow_dissectors()  earlier Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-22 20:20 +0100
                      Re: [PATCH net] flow_dissect: call init_default_flow_dissectors()  earlier Andre Noll <maan@tuebingen.mpg.de> - 2016-11-22 20:50 +0100
                      Re: [PATCH net] flow_dissect: call init_default_flow_dissectors()  earlier David Miller <davem@davemloft.net> - 2016-11-22 20:50 +0100
              Re: Linux 4.4.34 "Duyck, Alexander H" <alexander.h.duyck@intel.com> - 2016-11-22 19:10 +0100
              Re: Linux 4.4.34 Eric Dumazet <edumazet@google.com> - 2016-11-22 19:10 +0100
        Re: Linux 4.4.34 Andre Noll <maan@tuebingen.mpg.de> - 2016-11-22 19:00 +0100
      Re: Linux 4.4.34 Greg KH <gregkh@linuxfoundation.org> - 2016-11-22 18:30 +0100
      Re: Linux 4.4.34 Eric Dumazet <edumazet@google.com> - 2016-11-22 18:30 +0100
      Re: Linux 4.4.34 Andre Noll <maan@tuebingen.mpg.de> - 2016-11-22 19:10 +0100
    Re: Linux 4.4.34 Andre Noll <maan@tuebingen.mpg.de> - 2016-11-22 19:20 +0100

#1526482 — Linux 4.4.34

[Multipart message — attachments visible in raw view] — view raw

I'm announcing the release of the 4.4.34 kernel.

All users of the 4.4 kernel series must upgrade.

The updated 4.4.y git tree can be found at:
	git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git linux-4.4.y
and can be browsed at the normal kernel.org git web browser:
	http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=summary

thanks,

greg k-h

------------

 Makefile                              |    2 
 arch/sparc/include/asm/mmu_64.h       |    3 
 arch/sparc/include/asm/uaccess_64.h   |   66 ------
 arch/sparc/kernel/dtlb_prot.S         |    4 
 arch/sparc/kernel/head_64.S           |   36 ---
 arch/sparc/kernel/jump_label.c        |   23 +-
 arch/sparc/kernel/ktlb.S              |   12 +
 arch/sparc/kernel/sparc_ksyms_64.c    |    1 
 arch/sparc/kernel/tsb.S               |   12 -
 arch/sparc/lib/GENcopy_from_user.S    |    4 
 arch/sparc/lib/GENcopy_to_user.S      |    4 
 arch/sparc/lib/GENmemcpy.S            |   48 +++-
 arch/sparc/lib/Makefile               |    2 
 arch/sparc/lib/NG2copy_from_user.S    |    8 
 arch/sparc/lib/NG2copy_to_user.S      |    8 
 arch/sparc/lib/NG2memcpy.S            |  228 +++++++++++++-------
 arch/sparc/lib/NG4copy_from_user.S    |    8 
 arch/sparc/lib/NG4copy_to_user.S      |    8 
 arch/sparc/lib/NG4memcpy.S            |  294 ++++++++++++++++++++------
 arch/sparc/lib/NGcopy_from_user.S     |    4 
 arch/sparc/lib/NGcopy_to_user.S       |    4 
 arch/sparc/lib/NGmemcpy.S             |  233 ++++++++++++++-------
 arch/sparc/lib/U1copy_from_user.S     |    8 
 arch/sparc/lib/U1copy_to_user.S       |    8 
 arch/sparc/lib/U1memcpy.S             |  345 ++++++++++++++++++++-----------
 arch/sparc/lib/U3copy_from_user.S     |    8 
 arch/sparc/lib/U3copy_to_user.S       |    8 
 arch/sparc/lib/U3memcpy.S             |  227 ++++++++++++++------
 arch/sparc/lib/copy_in_user.S         |   35 ++-
 arch/sparc/lib/user_fixup.c           |   71 ------
 arch/sparc/mm/fault_64.c              |    6 
 arch/sparc/mm/hugetlbpage.c           |    4 
 arch/sparc/mm/init_64.c               |    3 
 arch/sparc/mm/tlb.c                   |    4 
 arch/sparc/mm/tsb.c                   |   31 ++
 arch/sparc/mm/ultra.S                 |  374 +++++++++++++++++++++++++++-------
 drivers/net/ethernet/broadcom/bgmac.c |    5 
 drivers/tty/serial/sunhv.c            |    6 
 drivers/tty/tty_ldisc.c               |    7 
 include/linux/filter.h                |    6 
 include/net/ip6_tunnel.h              |    1 
 include/net/tcp.h                     |    1 
 net/core/dev.c                        |    2 
 net/core/filter.c                     |   10 
 net/core/flow_dissector.c             |   11 -
 net/core/sock.c                       |    1 
 net/dccp/ipv4.c                       |   14 -
 net/dccp/ipv6.c                       |   16 -
 net/dccp/proto.c                      |    4 
 net/ipv4/fib_trie.c                   |   21 -
 net/ipv4/route.c                      |    4 
 net/ipv4/tcp.c                        |    2 
 net/ipv4/tcp_dctcp.c                  |   13 +
 net/ipv4/tcp_ipv4.c                   |   19 +
 net/ipv6/tcp_ipv6.c                   |    6 
 net/sctp/socket.c                     |    7 
 net/socket.c                          |    2 
 57 files changed, 1499 insertions(+), 803 deletions(-)

Alexander Duyck (1):
      fib_trie: Correct /proc/net/route off by one error

Andy Gospodarek (1):
      bgmac: stop clearing DMA receive control register right after it is set

Dan Carpenter (1):
      sparc: serial: sunhv: fix a double lock bug

David S. Miller (17):
      sparc: Don't leak context bits into thread->fault_address
      sparc64: Handle extremely large kernel TSB range flushes sanely.
      sparc64: Fix illegal relative branches in hypervisor patched TLB code.
      sparc64: Fix instruction count in comment for __hypervisor_flush_tlb_pending.
      sparc64: Fix illegal relative branches in hypervisor patched TLB cross-call code.
      sparc64: Handle extremely large kernel TLB range flushes more gracefully.
      sparc64: Delete __ret_efault.
      sparc64: Prepare to move to more saner user copy exception handling.
      sparc64: Convert copy_in_user to accurate exception reporting.
      sparc64: Convert GENcopy_{from,to}_user to accurate exception reporting.
      sparc64: Convert U1copy_{from,to}_user to accurate exception reporting.
      sparc64: Convert NG4copy_{from,to}_user to accurate exception reporting.
      sparc64: Convert NGcopy_{from,to}_user to accurate exception reporting.
      sparc64: Convert NG2copy_{from,to}_user to accurate exception reporting.
      sparc64: Convert U3copy_{from,to}_user to accurate exception reporting.
      sparc64: Delete now unused user copy assembler helpers.
      sparc64: Delete now unused user copy fixup functions.

Eli Cooper (1):
      ip6_tunnel: Clear IP6CB in ip6tunnel_xmit()

Eric Dumazet (9):
      net: clear sk_err_soft in sk_clone_lock()
      net: mangle zero checksum in skb_checksum_help()
      tcp: fix potential memory corruption
      dccp: do not send reset to already closed sockets
      dccp: fix out of bound access in dccp_v4_err()
      ipv6: dccp: fix out of bound access in dccp_v6_err()
      ipv6: dccp: add missing bind_conflict to dccp_ipv6_mapped
      net: __skb_flow_dissect() must cap its return value
      tcp: take care of truncations done by sk_filter()

Florian Westphal (1):
      dctcp: avoid bogus doubling of cwnd after loss

Greg Kroah-Hartman (1):
      Linux 4.4.34

James Clarke (1):
      sparc: Handle negative offsets in arch_jump_label_transform

Marcelo Ricardo Leitner (1):
      sctp: assign assoc_id earlier in __sctp_connect

Mike Kravetz (1):
      sparc64 mm: Fix base TSB sizing when hugetlb pages are used

Peter Hurley (1):
      tty: Prevent ldisc drivers from re-using stale tty fields

Soheil Hassas Yeganeh (1):
      sock: fix sendmmsg for partial sendmsg

Stephen Suryaputra Lin (1):
      ipv4: use new_gw for redirect neigh lookup

[toc] | [next] | [standalone]

#1527718

On Tue, Nov 22, 2016 at 05:59:12PM +0100, Andre Noll wrote:
> On Mon, Nov 21, 10:28, Greg KH wrote
> > I'm announcing the release of the 4.4.34 kernel.
> > 
> > All users of the 4.4 kernel series must upgrade.
> 
> This update broke PXE boot on our 4-way AMD boxes. The kernel panics in
> eth_type_trans(), presumably during kernel-level IP autoconfiguration,
> see [1]. Bisection points me at 5c67f947 (net: __skb_flow_dissect()
> must cap its return value). And indeed, reverting this commit fixes
> the problem for me.
> 
> Investigation showed that the real problem is not the change in the
> above commit per se (i.e., capping ->thoff) but the fact that in the
> success case, where we jump to the "out_good" label, ->thoff is now
> set *after* ->n_proto and ->ip_proto. I fail to see how order matters
> here, but it clearly does, since the crash is 100% reproducible,
> and is fixed by the commit below (on top of v4.4.34).
> 
> Please consider applying something like the patch below for mainline
> and -stable.

If this issue is also the same for Linus's tree, we should cc: netdev so
that the patch can get into there, right?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1527727

On Tue, Nov 22, 2016 at 9:06 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 22, 2016 at 05:59:12PM +0100, Andre Noll wrote:
>> On Mon, Nov 21, 10:28, Greg KH wrote
>> > I'm announcing the release of the 4.4.34 kernel.
>> >
>> > All users of the 4.4 kernel series must upgrade.
>>
>> This update broke PXE boot on our 4-way AMD boxes. The kernel panics in
>> eth_type_trans(), presumably during kernel-level IP autoconfiguration,
>> see [1]. Bisection points me at 5c67f947 (net: __skb_flow_dissect()
>> must cap its return value). And indeed, reverting this commit fixes
>> the problem for me.
>>
>> Investigation showed that the real problem is not the change in the
>> above commit per se (i.e., capping ->thoff) but the fact that in the
>> success case, where we jump to the "out_good" label, ->thoff is now
>> set *after* ->n_proto and ->ip_proto. I fail to see how order matters
>> here, but it clearly does, since the crash is 100% reproducible,
>> and is fixed by the commit below (on top of v4.4.34).
>>
>> Please consider applying something like the patch below for mainline
>> and -stable.
>
> If this issue is also the same for Linus's tree, we should cc: netdev so
> that the patch can get into there, right?
>
> thanks,
>
> greg k-h

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.

Adding a test in fast path looks overkill to me.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1527765

On Tue, 2016-11-22 at 09:14 -0800, Eric Dumazet wrote:
> On Tue, Nov 22, 2016 at 9:06 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > On Tue, Nov 22, 2016 at 05:59:12PM +0100, Andre Noll wrote:
> > > 
> > > On Mon, Nov 21, 10:28, Greg KH wrote
> > > > 
> > > > I'm announcing the release of the 4.4.34 kernel.
> > > > 
> > > > All users of the 4.4 kernel series must upgrade.
> > > 
> > > This update broke PXE boot on our 4-way AMD boxes. The kernel panics in
> > > eth_type_trans(), presumably during kernel-level IP autoconfiguration,
> > > see [1]. Bisection points me at 5c67f947 (net: __skb_flow_dissect()
> > > must cap its return value). And indeed, reverting this commit fixes
> > > the problem for me.
> > > 
> > > Investigation showed that the real problem is not the change in the
> > > above commit per se (i.e., capping ->thoff) but the fact that in the
> > > success case, where we jump to the "out_good" label, ->thoff is now
> > > set *after* ->n_proto and ->ip_proto. I fail to see how order matters
> > > here, but it clearly does, since the crash is 100% reproducible,
> > > and is fixed by the commit below (on top of v4.4.34).
> > > 
> > > Please consider applying something like the patch below for mainline
> > > and -stable.
> > 
> > If this issue is also the same for Linus's tree, we should cc: netdev so
> > that the patch can get into there, right?
> > 
> > thanks,
> > 
> > greg k-h
> 
> 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.
> 
> Adding a test in fast path looks overkill to me.
> 
> Thanks.

I was wondering if we shouldn't just cap all cases?

It seems like this could potentially return a value greater than skb-
>len in the "good" case since things like IP header length isn't
validated other then making sure it meets the minimum value, and if
there isn't a recognized L4 header after that we could return that as a
final value.

- Alex

[toc] | [prev] | [next] | [standalone]

#1527775

.
>
> I was wondering if we shouldn't just cap all cases?
>
> It seems like this could potentially return a value greater than skb-
>>len in the "good" case since things like IP header length isn't
> validated other then making sure it meets the minimum value, and if
> there isn't a recognized L4 header after that we could return that as a
> final value.
>

I honestly think you are looking at the wrong causes.

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): {

[toc] | [prev] | [next] | [standalone]

#1527787

[Multipart message — attachments visible in raw view] — view raw

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

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/

[toc] | [prev] | [next] | [standalone]

#1527803

[Multipart message — attachments visible in raw view] — view raw

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/

[toc] | [prev] | [next] | [standalone]

#1527811

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

[toc] | [prev] | [next] | [standalone]

#1527828

On Tue, Nov 22, 2016 at 10:08 AM, Duyck, Alexander H
<alexander.h.duyck@intel.com> 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.
>

Oh that might be that, goot catch !

Time to try :

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);

[toc] | [prev] | [next] | [standalone]

#1527833

[Multipart message — attachments visible in raw view] — view raw

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/

[toc] | [prev] | [next] | [standalone]

#1527858 — [PATCH net] flow_dissect: call init_default_flow_dissectors() earlier

From: Eric Dumazet <edumazet@google.com>

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 <edumazet@google.com>
Reported-by: Andre Noll <maan@tuebingen.mpg.de>
Diagnosed-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 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);

[toc] | [prev] | [next] | [standalone]

#1527878 — Re: [PATCH net] flow_dissect: call init_default_flow_dissectors() earlier

[Multipart message — attachments visible in raw view] — view raw

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 <maan@tuebingen.mpg.de>

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/

[toc] | [prev] | [next] | [standalone]

#1527880 — Re: [PATCH net] flow_dissect: call init_default_flow_dissectors() earlier

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 22 Nov 2016 11:17:30 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> 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 <edumazet@google.com>
> Reported-by: Andre Noll <maan@tuebingen.mpg.de>
> Diagnosed-by: Alexander Duyck <alexander.h.duyck@intel.com>

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

[toc] | [prev] | [next] | [standalone]

#1527804

On Tue, 2016-11-22 at 09:56 -0800, Eric Dumazet wrote:
> On Tue, Nov 22, 2016 at 9:55 AM, Andre Noll <maan@tuebingen.mpg.de> 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

[toc] | [prev] | [next] | [standalone]

#1527810

On Tue, Nov 22, 2016 at 9:55 AM, Andre Noll <maan@tuebingen.mpg.de> 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;

[toc] | [prev] | [next] | [standalone]

#1527798

[Multipart message — attachments visible in raw view] — view raw

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/

[toc] | [prev] | [next] | [standalone]

#1527735

On Tue, Nov 22, 2016 at 06:22:47PM +0100, Andre Noll wrote:
> On Tue, Nov 22, 18:06, Greg KH wrote
> > On Tue, Nov 22, 2016 at 05:59:12PM +0100, Andre Noll wrote:
> > > On Mon, Nov 21, 10:28, Greg KH wrote
> > > > I'm announcing the release of the 4.4.34 kernel.
> > > > 
> > > > All users of the 4.4 kernel series must upgrade.
> > > 
> > > This update broke PXE boot on our 4-way AMD boxes. The kernel panics in
> > > eth_type_trans(), presumably during kernel-level IP autoconfiguration,
> > > see [1]. Bisection points me at 5c67f947 (net: __skb_flow_dissect()
> > > must cap its return value). And indeed, reverting this commit fixes
> > > the problem for me.
> > > 
> > > Investigation showed that the real problem is not the change in the
> > > above commit per se (i.e., capping ->thoff) but the fact that in the
> > > success case, where we jump to the "out_good" label, ->thoff is now
> > > set *after* ->n_proto and ->ip_proto. I fail to see how order matters
> > > here, but it clearly does, since the crash is 100% reproducible,
> > > and is fixed by the commit below (on top of v4.4.34).
> > > 
> > > Please consider applying something like the patch below for mainline
> > > and -stable.
> > 
> > If this issue is also the same for Linus's tree, we should cc: netdev so
> > that the patch can get into there, right?
> 
> Right, but I haven't tested PXE boot on any kernel newer than 4.4.x
> so far.  All I can say for sure is that the problematic commit is
> also in Linus' tree (called 34fad54c there).
> 
> Do you want me to check if mainline is also affected?

That would be great if you could do so.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1527742

On Tue, Nov 22, 2016 at 9:22 AM, Andre Noll <maan@tuebingen.mpg.de> wrote:
> On Tue, Nov 22, 18:06, Greg KH wrote
>> On Tue, Nov 22, 2016 at 05:59:12PM +0100, Andre Noll wrote:
>> > On Mon, Nov 21, 10:28, Greg KH wrote
>> > > I'm announcing the release of the 4.4.34 kernel.
>> > >
>> > > All users of the 4.4 kernel series must upgrade.
>> >
>> > This update broke PXE boot on our 4-way AMD boxes. The kernel panics in
>> > eth_type_trans(), presumably during kernel-level IP autoconfiguration,
>> > see [1]. Bisection points me at 5c67f947 (net: __skb_flow_dissect()
>> > must cap its return value). And indeed, reverting this commit fixes
>> > the problem for me.
>> >
>> > Investigation showed that the real problem is not the change in the
>> > above commit per se (i.e., capping ->thoff) but the fact that in the
>> > success case, where we jump to the "out_good" label, ->thoff is now
>> > set *after* ->n_proto and ->ip_proto. I fail to see how order matters
>> > here, but it clearly does, since the crash is 100% reproducible,
>> > and is fixed by the commit below (on top of v4.4.34).
>> >
>> > Please consider applying something like the patch below for mainline
>> > and -stable.
>>
>> If this issue is also the same for Linus's tree, we should cc: netdev so
>> that the patch can get into there, right?
>
> Right, but I haven't tested PXE boot on any kernel newer than 4.4.x
> so far.  All I can say for sure is that the problematic commit is
> also in Linus' tree (called 34fad54c there).
>
> Do you want me to check if mainline is also affected?
>

Mainline is affected, we had a report of someone using IGB and hitting the bug.

We now have a hint based on your patch, thanks !

[toc] | [prev] | [next] | [standalone]

#1527819

[Multipart message — attachments visible in raw view] — view raw

On Tue, Nov 22, 18:06, Greg KH wrote
> On Tue, Nov 22, 2016 at 05:59:12PM +0100, Andre Noll wrote:
> > On Mon, Nov 21, 10:28, Greg KH wrote
> > > I'm announcing the release of the 4.4.34 kernel.
> > > 
> > > All users of the 4.4 kernel series must upgrade.
> > 
> > This update broke PXE boot on our 4-way AMD boxes. The kernel panics in
> > eth_type_trans(), presumably during kernel-level IP autoconfiguration,
> > see [1]. Bisection points me at 5c67f947 (net: __skb_flow_dissect()
> > must cap its return value). And indeed, reverting this commit fixes
> > the problem for me.
> > 
> > Investigation showed that the real problem is not the change in the
> > above commit per se (i.e., capping ->thoff) but the fact that in the
> > success case, where we jump to the "out_good" label, ->thoff is now
> > set *after* ->n_proto and ->ip_proto. I fail to see how order matters
> > here, but it clearly does, since the crash is 100% reproducible,
> > and is fixed by the commit below (on top of v4.4.34).
> > 
> > Please consider applying something like the patch below for mainline
> > and -stable.
> 
> If this issue is also the same for Linus's tree, we should cc: netdev so
> that the patch can get into there, right?

Right, but I haven't tested PXE boot on any kernel newer than 4.4.x
so far.  All I can say for sure is that the problematic commit is
also in Linus' tree (called 34fad54c there).

Do you want me to check if mainline is also affected?

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/

[toc] | [prev] | [next] | [standalone]

#1527826

[Multipart message — attachments visible in raw view] — view raw

On Mon, Nov 21, 10:28, Greg KH wrote
> I'm announcing the release of the 4.4.34 kernel.
> 
> All users of the 4.4 kernel series must upgrade.

This update broke PXE boot on our 4-way AMD boxes. The kernel panics in
eth_type_trans(), presumably during kernel-level IP autoconfiguration,
see [1]. Bisection points me at 5c67f947 (net: __skb_flow_dissect()
must cap its return value). And indeed, reverting this commit fixes
the problem for me.

Investigation showed that the real problem is not the change in the
above commit per se (i.e., capping ->thoff) but the fact that in the
success case, where we jump to the "out_good" label, ->thoff is now
set *after* ->n_proto and ->ip_proto. I fail to see how order matters
here, but it clearly does, since the crash is 100% reproducible,
and is fixed by the commit below (on top of v4.4.34).

Please consider applying something like the patch below for mainline
and -stable.

Thanks
Andre

[1] http://people.tuebingen.mpg.de/maan/.eth_type_trans_panic.png
---
commit 3494d99764da2b7bbac5d0eed364d03552fefd36
Author: Andre Noll <maan@tuebingen.mpg.de>
Date:   Tue Nov 22 11:34:11 2016 +0100

    net: __skb_flow_dissect() must set ->thoff *after* ->n_proto and ->ip_proto
    
    Without this, PXE boot fails on some AMD Opteron machines.
    
    Signed-off-by: Andre Noll <maan@tuebingen.mpg.de>

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 9aba9e9..fde26e4 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -491,18 +491,17 @@ ip_proto_again:
 
 out_good:
 	ret = true;
-
-	key_control->thoff = (u16)nhoff;
+	goto out;
+out_bad:
+	ret = false;
 out:
 	key_basic->n_proto = proto;
 	key_basic->ip_proto = ip_proto;
-
+	if (ret)
+		key_control->thoff = (u16)nhoff;
+	else
+		key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
 	return ret;
-
-out_bad:
-	ret = false;
-	key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
-	goto out;
 }
 EXPORT_SYMBOL(__skb_flow_dissect);
-- 
Max Planck Institute for Developmental Biology
Spemannstraße 35, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web