On Tue, Nov 14, 2017 at 12:05:46PM +0100, Florian Westphal wrote:
Sebastian Gottschall s.gottschall@dd-wrt.com wrote:
this should belong to you. with the revert the kernel crashes for me
[ 24.838120] Unable to handle kernel NULL pointer dereference at virtual address 00000055 [ 24.846193] pgd = c0004000 [ 24.848893] [00000055] *pgd=00000000 [ 24.852472] Internal error: Oops - BUG: 817 [#1] PREEMPT SMP ARM [ 24.858463] Modules linked in: xhci_plat_hcd xhci_pci xhci_hcd ohci_hcd ehci_pci ehci_platform ehci_hcd usbcore usb_common nls_base qca_ssdk gpio_pca953x mii_gpio wil6210 ath10k_pci ath10k_core ath9k ath9k_common ath9k_hw ath mac80211 cfg80211 compat [ 24.880852] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.9.62-rc1 #90 [ 24.887189] Hardware name: AnnapurnaLabs Alpine (Device Tree) [ 24.892921] task: ef029ac0 task.stack: ef05a000 [ 24.897444] PC is at nf_nat_cleanup_conntrack+0x4c/0x74 [ 24.902657] LR is at nf_nat_cleanup_conntrack+0x38/0x74 [ 24.907869] pc : [<c04858c8>] lr : [<c04858b4>] psr: 60000153 [ 24.907869] sp : ef05bb58 ip : ef05bb58 fp : ef05bb6c [ 24.919317] r10: ed230cc0 r9 : ed230c00 r8 : edf45800 [ 24.924529] r7 : ebcd2f00 r6 : ec33739e r5 : c0892294 r4 : ebcd2f00 [ 24.931040] r3 : 00000000 r2 : 00000055 r1 : 00000000 r0 : c0892718 [ 24.937551] Flags: nZCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment user [ 24.944755] Control: 10c5387d Table: 2bd1006a DAC: 00000055 [ 24.950486] Process swapper/2 (pid: 0, stack limit = 0xef05a210) [ 24.956477] Stack: (0xef05bb58 to 0xef05c000)
Guillaume, can you check what I missed? IIRC you said you had a revert for your 4.9 tree, correct?
Actually I had forgotten some details when I told you about it. What I did is simpler than a revert. I just cherry-picked the following commits: 124dffea9e8e ("netfilter: nat: use atomic bit op to clear the _SRC_NAT_DONE_BIT") 6e699867f84c ("netfilter: nat: avoid use of nf_conn_nat extension") e1bf1687740c ("netfilter: nat: Revert "netfilter: nat: convert nat bysrc hash to rhashtable"")
Then I just had to fix one remaining call: @@ -842,7 +842,7 @@ static int __init nf_nat_init(void) return 0;
cleanup_extend: - rhltable_destroy(&nf_nat_bysource_table); + nf_ct_free_hashtable(nf_nat_bysource, nf_nat_htable_size); nf_ct_extend_unregister(&nat_extend); return ret; }
My original approach was to revert commit 870190a9ec90 ("netfilter: nat: convert nat bysrc hash to rhashtable") and fix the conflicts by hand. But I had crashes when testing the result. After a bit of debugging, I decided to simply try cherry-picking commit e1bf1687740c and its dependencies. Since I got better results with this approach, I dropped my original revert code (so, unfortunately, I can't compare with yours and I don't remember if the crash was similar to Sebastian's). We have the cherry-picked patches running on several machines since September. No problem found so far.
Now, looking at Sebastian's report, I suspect that using nf_ct_ext_find(NF_CT_EXT_NAT) in nf_nat_cleanup_conntrack() might not be appropriate anymore. My approach uses the IPS_SRC_NAT_DONE bit instead, which is now set atomically. Theses changes weren't intentional, but introduced by the dependency on commits 124dffea9e8e and 6e699867f84c.
Does that make sense to you? I can look more deeply into this tomorrow.