Bug 220078

Summary: [patch] [panic] repeatable kernel panic due to unlocked INADDR_TO_IFP usage
Product: Base System Reporter: Eugene Grosbein <eugen>
Component: kernAssignee: Eugene Grosbein <eugen>
Status: Closed FIXED    
Severity: Affects Some People CC: cem, laa88rf, net, peixoto.cassiano, pi, ports
Priority: --- Keywords: crash
Version: 11.0-STABLEFlags: eugen: mfc-stable12+
koobs: mfc-stable11+
eugen: mfc-stable10+
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=186114
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=220140
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=222617
https://reviews.freebsd.org/D12457
Attachments:
Description Flags
lock access to INADDR_TO_IFP in the ipfw
none
lock access to INADDR_TO_IFP in the in_mcast.c
none
lock access to INADDR_HASH in the stf(4)
none
lock access to INADDR_HASH in the ip_input()
none
Proposed patch (untested)
none
stable/10: lock access to INADDR_HASH in the ip_input()
none
stable/10: lock access to INADDR_TO_IFP in the in_mcast.c none

Description Eugene Grosbein freebsd_committer 2017-06-17 12:18:07 UTC
Created attachment 183567 [details]
lock access to INADDR_TO_IFP in the ipfw

There are several places in kernel sources where code performs unlocked access to the hash of host's IP addresses. If adresses change often enough, a panic may occur.

For example, part of kgdb script of latest panic of my mpd server:

1642                            case O_IP_DST_ME:
1643                                    if (is_ipv4) {
1644                                            struct ifnet *tif;
1645
1646                                            INADDR_TO_IFP(dst_ip, tif);
(kgdb) p ia
$3 = (struct in_ifaddr *) 0xdeadc0dedeadc0de

Attached patches add needed locking to several such places including ipfw.
Comment 1 Eugene Grosbein freebsd_committer 2017-06-17 12:19:08 UTC
Created attachment 183568 [details]
lock access to INADDR_TO_IFP in the in_mcast.c
Comment 2 Eugene Grosbein freebsd_committer 2017-06-17 12:19:55 UTC
Created attachment 183569 [details]
lock access to INADDR_HASH in the stf(4)
Comment 3 Eugene Grosbein freebsd_committer 2017-06-17 12:22:14 UTC
Created attachment 183570 [details]
lock access to INADDR_HASH in the ip_input()
Comment 4 Andrey V. Elsukov freebsd_committer 2017-06-18 06:09:19 UTC
Created attachment 183590 [details]
Proposed patch (untested)

For ipfw(4) I prefer to use this patch. I believe it has a bit less overhead. Can you test it?
Comment 5 Eugene Grosbein freebsd_committer 2017-06-18 12:02:45 UTC
(In reply to Andrey V. Elsukov from comment #4)

I've just rebuilt the kernel with your version of patch instead of old one and restarted my tests. New version does basically same thing, so I do not expect problems. The system exhibited no problems at all during a day with my stress test using all patches involving (for kernel, libc and mpd itself). Unless something bad happens next day, I'm going to put it in "testing production".
Comment 6 Cassiano Peixoto 2017-06-19 20:11:38 UTC
(In reply to Eugene Grosbein from comment #5)
Eugene, should i apply this one too?
Comment 7 Eugene Grosbein freebsd_committer 2017-06-19 20:51:17 UTC
(In reply to Cassiano Peixoto from comment #6)

This PR presents several patches for several kernel subsystems.

Apply patches for ip_mcast.c and ip_input().

The patch for ipfw is only needed if you use ipfw and have a rule with "me" keyword like "allow ip from any to me". Better use version by ae@ (https://bugs.freebsd.org/bugzilla/attachment.cgi?id=183590).

The patch for stf(4) is only needed if you use stf(4).
Comment 8 Cassiano Peixoto 2017-06-19 20:55:21 UTC
(In reply to Eugene Grosbein from comment #7)
Do i need to recompile the world or only the kernel?
Comment 9 Eugene Grosbein freebsd_committer 2017-06-19 20:57:47 UTC
(In reply to Cassiano Peixoto from comment #8)

Kernel only.
Comment 10 Cassiano Peixoto 2017-06-19 21:04:23 UTC
(In reply to Eugene Grosbein from comment #9)
mcast has been reject:

# patch < mcast.patch 
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- sys/netinet/in_mcast.c.orig	2017-04-20 15:01:10.786840000 +0700
|+++ sys/netinet/in_mcast.c	2017-06-17 18:24:34.034823000 +0700
--------------------------
Patching file sys/netinet/in_mcast.c using Plan A...
Hunk #1 succeeded at 1340 (offset 2 lines).
Hunk #2 succeeded at 1378 (offset 2 lines).
Hunk #3 succeeded at 1878 (offset 2 lines).
Hunk #4 succeeded at 1895 (offset 2 lines).
Hunk #5 succeeded at 2229 (offset 2 lines).
Hunk #6 failed at 2288.
Hunk #7 succeeded at 2452 (offset 2 lines).
Hunk #8 succeeded at 2491 (offset 2 lines).
1 out of 8 hunks failed--saving rejects to sys/netinet/in_mcast.c.rej
done
Exit 1

# cat sys/netinet/in_mcast.c.rej
@@ -2283,9 +2288,11 @@
 		 * XXX NOTE WELL: The RFC 3678 API is preferred because
 		 * using an IPv4 address as a key is racy.
 		 */
-		if (!in_nullhost(mreqs.imr_interface))
+		if (!in_nullhost(mreqs.imr_interface)) {
+			IN_IFADDR_RLOCK(&in_ifa_tracker);
 			INADDR_TO_IFP(mreqs.imr_interface, ifp);
-
+			IN_IFADDR_RUNLOCK(&in_ifa_tracker);
+		}
 		CTR3(KTR_IGMPV3, "%s: imr_interface = 0x%08x, ifp = %p",
 		    __func__, ntohl(mreqs.imr_interface.s_addr), ifp);
Comment 11 Eugene Grosbein freebsd_committer 2017-06-19 21:44:49 UTC
(In reply to Cassiano Peixoto from comment #10)

You have revision earlier than 315456 that changed context for this patch:

-		CTR3(KTR_IGMPV3, "%s: imr_interface = %s, ifp = %p",
-		    __func__, inet_ntoa(mreqs.imr_interface), ifp);
+		CTR3(KTR_IGMPV3, "%s: imr_interface = 0x%08x, ifp = %p",
+		    __func__, ntohl(mreqs.imr_interface.s_addr), ifp);

That's not big deal. Either apply this small chunk manually, or update your system to latest stable/11.
Comment 12 Eugene Grosbein freebsd_committer 2017-06-19 21:47:18 UTC
(In reply to Cassiano Peixoto from comment #10)

You can also just skip this patch for now if your system does not process any multicast traffic including routing protocols like OSPF or RIPv2.
Comment 13 Cassiano Peixoto 2017-06-20 12:18:24 UTC
(In reply to Eugene Grosbein from comment #11)
There is a reference for CTR3() 3 times in the code. Should i replace all of them?

Thanks.
Comment 14 Eugene Grosbein freebsd_committer 2017-06-20 12:25:52 UTC
(In reply to Cassiano Peixoto from comment #13)

You do not need to change context of the patch. That is, do not touch lines referring to CTR3. Just make supposed change manually, e.g. add lines with IN_IFADDR_RLOCK/IN_IFADDR_RUNLOCK and fix braces.
Comment 15 Cassiano Peixoto 2017-06-20 13:46:55 UTC
(In reply to Eugene Grosbein from comment #14)
Ok got it. Patch applied fine. Thanks.
Comment 16 Eugene Grosbein freebsd_committer 2017-06-27 19:38:46 UTC
(In reply to Andrey V. Elsukov from comment #4)

Andrey, there is no problems with your patch for ipfw. Please commit.
Comment 17 Eugene Grosbein freebsd_committer 2017-09-19 18:01:38 UTC
*** Bug 162558 has been marked as a duplicate of this bug. ***
Comment 18 commit-hook freebsd_committer 2017-09-20 22:35:51 UTC
A commit references this bug:

Author: ae
Date: Wed Sep 20 22:35:28 UTC 2017
New revision: 323839
URL: https://svnweb.freebsd.org/changeset/base/323839

Log:
  Use in_localip() function instead of unlocked access to addresses hash
  to determine that an address is our local.

  PR:		220078
  MFC after:	1 week

Changes:
  head/sys/netpfil/ipfw/ip_fw2.c
Comment 19 Eugene Grosbein freebsd_committer 2017-09-22 08:09:20 UTC
My PR.
Comment 20 commit-hook freebsd_committer 2017-09-27 01:48:15 UTC
A commit references this bug:

Author: ae
Date: Wed Sep 27 01:47:54 UTC 2017
New revision: 324047
URL: https://svnweb.freebsd.org/changeset/base/324047

Log:
  MFC r323839:
    Use in_localip() function instead of unlocked access to addresses hash
    to determine that an address is our local.

    PR:		220078

Changes:
_U  stable/11/
  stable/11/sys/netpfil/ipfw/ip_fw2.c
Comment 21 Eugene Grosbein freebsd_committer 2017-11-03 10:31:01 UTC
Created attachment 187688 [details]
stable/10: lock access to INADDR_HASH in the ip_input()

Fix for ip_input() adjusted to stable/10 code base.
Comment 22 Eugene Grosbein freebsd_committer 2017-11-03 10:35:57 UTC
Created attachment 187689 [details]
stable/10: lock access to INADDR_TO_IFP in the in_mcast.c

Fix for in_mcast.c adjusted to stable/10 code base.
Comment 23 Eugene Grosbein freebsd_committer 2018-02-05 17:07:37 UTC
*** Bug 222617 has been marked as a duplicate of this bug. ***
Comment 24 Eugene Grosbein freebsd_committer 2018-02-05 17:20:35 UTC
Also https://reviews.freebsd.org/D12457
Comment 25 Eugene Grosbein freebsd_committer 2018-10-21 22:24:20 UTC
*** Bug 195102 has been marked as a duplicate of this bug. ***
Comment 26 commit-hook freebsd_committer 2018-10-27 04:45:34 UTC
A commit references this bug:

Author: eugen
Date: Sat Oct 27 04:45:28 UTC 2018
New revision: 339806
URL: https://svnweb.freebsd.org/changeset/base/339806

Log:
  Prevent stf(4) from panicing due to unprotected access to INADDR_HASH.

  PR:			220078
  MFC after:		1 month
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
  head/sys/net/if_stf.c
Comment 27 commit-hook freebsd_committer 2018-10-27 04:53:46 UTC
A commit references this bug:

Author: eugen
Date: Sat Oct 27 04:53:26 UTC 2018
New revision: 339807
URL: https://svnweb.freebsd.org/changeset/base/339807

Log:
  Prevent multicast code from panicing due to unprotected access to INADDR_HASH.

  PR:			220078
  MFC after:		1 month
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
  head/sys/netinet/in_mcast.c
Comment 28 commit-hook freebsd_committer 2018-10-27 04:59:53 UTC
A commit references this bug:

Author: eugen
Date: Sat Oct 27 04:59:36 UTC 2018
New revision: 339808
URL: https://svnweb.freebsd.org/changeset/base/339808

Log:
  Prevent ip_input() from panicing due to unprotected access to INADDR_HASH.

  PR:			220078
  MFC after:		1 month
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
  head/sys/netinet/ip_input.c
Comment 29 commit-hook freebsd_committer 2018-11-26 10:47:26 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 10:47:01 UTC 2018
New revision: 340934
URL: https://svnweb.freebsd.org/changeset/base/340934

Log:
  MFC r339807: Prevent multicast code from panicing due to unprotected access to INADDR_HASH.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
_U  stable/12/
  stable/12/sys/netinet/in_mcast.c
Comment 30 commit-hook freebsd_committer 2018-11-26 10:51:33 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 10:50:50 UTC 2018
New revision: 340935
URL: https://svnweb.freebsd.org/changeset/base/340935

Log:
  MFC r339807: Prevent multicast code from panicing due to unprotected access
  to INADDR_HASH.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
_U  stable/11/
  stable/11/sys/netinet/in_mcast.c
Comment 31 commit-hook freebsd_committer 2018-11-26 11:16:58 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 11:15:59 UTC 2018
New revision: 340950
URL: https://svnweb.freebsd.org/changeset/base/340950

Log:
  MFC r339806: Prevent stf(4) from panicing due to unprotected access
  to INADDR_HASH.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
_U  stable/12/
  stable/12/sys/net/if_stf.c
Comment 32 commit-hook freebsd_committer 2018-11-26 11:18:02 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 11:17:12 UTC 2018
New revision: 340951
URL: https://svnweb.freebsd.org/changeset/base/340951

Log:
  MFC r339806: Prevent stf(4) from panicing due to unprotected access
  to INADDR_HASH.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
_U  stable/11/
  stable/11/sys/net/if_stf.c
Comment 33 commit-hook freebsd_committer 2018-11-26 11:52:36 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 11:51:44 UTC 2018
New revision: 340957
URL: https://svnweb.freebsd.org/changeset/base/340957

Log:
  Prevent multicast code from panicing due to unprotected access
  to INADDR_HASH. This is direct commit to stable/10 instead of MFC r339807
  due to significant difference in code base.

  PR:                   		220078
  Tested-by:			Cassiano Peixoto and others

Changes:
  stable/10/sys/netinet/in_mcast.c
Comment 34 commit-hook freebsd_committer 2018-11-26 12:19:59 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 12:19:31 UTC 2018
New revision: 340958
URL: https://svnweb.freebsd.org/changeset/base/340958

Log:
  Prevent stf(4) from panicing due to unprotected access
  to INADDR_HASH. This is direct commit to stable/10
  instead of MFC r339806 due to significant differences
  in code base.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
  stable/10/sys/net/if_stf.c
Comment 35 commit-hook freebsd_committer 2018-11-26 12:40:17 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 12:39:57 UTC 2018
New revision: 340959
URL: https://svnweb.freebsd.org/changeset/base/340959

Log:
  MFC r339808: Prevent ip_input() from panicing due to unprotected access
  to INADDR_HASH.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
_U  stable/12/
  stable/12/sys/netinet/ip_input.c
Comment 36 commit-hook freebsd_committer 2018-11-26 12:42:21 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 12:41:50 UTC 2018
New revision: 340960
URL: https://svnweb.freebsd.org/changeset/base/340960

Log:
  MFC r339808: Prevent ip_input() from panicing due to unprotected access
  to INADDR_HASH.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
_U  stable/11/
  stable/11/sys/netinet/ip_input.c
Comment 37 commit-hook freebsd_committer 2018-11-26 12:47:27 UTC
A commit references this bug:

Author: eugen
Date: Mon Nov 26 12:47:13 UTC 2018
New revision: 340961
URL: https://svnweb.freebsd.org/changeset/base/340961

Log:
  Prevent ip_input() from panicing due to unprotected access
  to INADDR_HASH. This is direct commit to stable/10 instead of MFC r339808
  due to significant differences in code base.

  PR:			220078
  Differential Revision:	https://reviews.freebsd.org/D12457
  Tested-by:		Cassiano Peixoto and others

Changes:
  stable/10/sys/netinet/ip_input.c
Comment 38 Eugene Grosbein freebsd_committer 2018-11-26 12:49:30 UTC
Fixed and merged to all supported branches: stable/10, stable/11 and stable/12.