lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 31 Mar 2022 20:34:42 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org
CC:     donaldsharp72@...il.com, philippe.guibert@...look.com,
        kuba@...nel.org, davem@...emloft.net, idosch@...sch.org
Subject: Re: [PATCH net 1/2] net: ipv4: fix route with nexthop object delete warning

On 31 March 2022 20:17:14 EEST, Nikolay Aleksandrov <razor@...ckwall.org> wrote:
>On 31 March 2022 20:05:43 EEST, David Ahern <dsahern@...nel.org> wrote:
>>On 3/31/22 9:46 AM, Nikolay Aleksandrov wrote:
>>> FRR folks have hit a kernel warning[1] while deleting routes[2] which is
>>> caused by trying to delete a route pointing to a nexthop id without
>>> specifying nhid but matching on an interface. That is, a route is found
>>> but we hit a warning while matching it. The warning is from
>>> fib_info_nh() in include/net/nexthop.h because we run it on a fib_info
>>> with nexthop object. The call chain is:
>>>  inet_rtm_delroute -> fib_table_delete -> fib_nh_match (called with a
>>> nexthop fib_info and also with fc_oif set thus calling fib_info_nh on
>>> the fib_info and triggering the warning). The fix is to not do any
>>> matching in that branch if the fi has a nexthop object because those are
>>> managed separately.
>>> 
>>> [1]
>>>  [  523.462226] ------------[ cut here ]------------
>>>  [  523.462230] WARNING: CPU: 14 PID: 22893 at include/net/nexthop.h:468 fib_nh_match+0x210/0x460
>>>  [  523.462236] Modules linked in: dummy rpcsec_gss_krb5 xt_socket nf_socket_ipv4 nf_socket_ipv6 ip6table_raw iptable_raw bpf_preload xt_statistic ip_set ip_vs_sh ip_vs_wrr ip_vs_rr ip_vs xt_mark nf_tables xt_nat veth nf_conntrack_netlink nfnetlink xt_addrtype br_netfilter overlay dm_crypt nfsv3 nfs fscache netfs vhost_net vhost vhost_iotlb tap tun xt_CHECKSUM xt_MASQUERADE xt_conntrack 8021q garp mrp ipt_REJECT nf_reject_ipv4 ip6table_mangle ip6table_nat iptable_mangle iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_filter bridge stp llc rfcomm snd_seq_dummy snd_hrtimer rpcrdma rdma_cm iw_cm ib_cm ib_core ip6table_filter xt_comment ip6_tables vboxnetadp(OE) vboxnetflt(OE) vboxdrv(OE) qrtr bnep binfmt_misc xfs vfat fat squashfs loop nvidia_drm(POE) nvidia_modeset(POE) nvidia_uvm(POE) nvidia(POE) intel_rapl_msr intel_rapl_common snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi btusb btrtl iwlmvm uvcvideo btbcm snd_hda_intel edac_mce_amd
>>>  [  523.462274]  videobuf2_vmalloc videobuf2_memops btintel snd_intel_dspcfg videobuf2_v4l2 snd_intel_sdw_acpi bluetooth snd_usb_audio snd_hda_codec mac80211 snd_usbmidi_lib joydev snd_hda_core videobuf2_common kvm_amd snd_rawmidi snd_hwdep snd_seq videodev ccp snd_seq_device libarc4 ecdh_generic mc snd_pcm kvm iwlwifi snd_timer drm_kms_helper snd cfg80211 cec soundcore irqbypass rapl wmi_bmof i2c_piix4 rfkill k10temp pcspkr acpi_cpufreq nfsd auth_rpcgss nfs_acl lockd grace sunrpc drm zram ip_tables crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel nvme sp5100_tco r8169 nvme_core wmi ipmi_devintf ipmi_msghandler fuse
>>>  [  523.462300] CPU: 14 PID: 22893 Comm: ip Tainted: P           OE     5.16.18-200.fc35.x86_64 #1
>>>  [  523.462302] Hardware name: Micro-Star International Co., Ltd. MS-7C37/MPG X570 GAMING EDGE WIFI (MS-7C37), BIOS 1.C0 10/29/2020
>>>  [  523.462303] RIP: 0010:fib_nh_match+0x210/0x460
>>>  [  523.462304] Code: 7c 24 20 48 8b b5 90 00 00 00 e8 bb ee f4 ff 48 8b 7c 24 20 41 89 c4 e8 ee eb f4 ff 45 85 e4 0f 85 2e fe ff ff e9 4c ff ff ff <0f> 0b e9 17 ff ff ff 3c 0a 0f 85 61 fe ff ff 48 8b b5 98 00 00 00
>>>  [  523.462306] RSP: 0018:ffffaa53d4d87928 EFLAGS: 00010286
>>>  [  523.462307] RAX: 0000000000000000 RBX: ffffaa53d4d87a90 RCX: ffffaa53d4d87bb0
>>>  [  523.462308] RDX: ffff9e3d2ee6be80 RSI: ffffaa53d4d87a90 RDI: ffffffff920ed380
>>>  [  523.462309] RBP: ffff9e3d2ee6be80 R08: 0000000000000064 R09: 0000000000000000
>>>  [  523.462310] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000031
>>>  [  523.462310] R13: 0000000000000020 R14: 0000000000000000 R15: ffff9e3d331054e0
>>>  [  523.462311] FS:  00007f245517c1c0(0000) GS:ffff9e492ed80000(0000) knlGS:0000000000000000
>>>  [  523.462313] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>  [  523.462313] CR2: 000055e5dfdd8268 CR3: 00000003ef488000 CR4: 0000000000350ee0
>>>  [  523.462315] Call Trace:
>>>  [  523.462316]  <TASK>
>>>  [  523.462320]  fib_table_delete+0x1a9/0x310
>>>  [  523.462323]  inet_rtm_delroute+0x93/0x110
>>>  [  523.462325]  rtnetlink_rcv_msg+0x133/0x370
>>>  [  523.462327]  ? _copy_to_iter+0xb5/0x6f0
>>>  [  523.462330]  ? rtnl_calcit.isra.0+0x110/0x110
>>>  [  523.462331]  netlink_rcv_skb+0x50/0xf0
>>>  [  523.462334]  netlink_unicast+0x211/0x330
>>>  [  523.462336]  netlink_sendmsg+0x23f/0x480
>>>  [  523.462338]  sock_sendmsg+0x5e/0x60
>>>  [  523.462340]  ____sys_sendmsg+0x22c/0x270
>>>  [  523.462341]  ? import_iovec+0x17/0x20
>>>  [  523.462343]  ? sendmsg_copy_msghdr+0x59/0x90
>>>  [  523.462344]  ? __mod_lruvec_page_state+0x85/0x110
>>>  [  523.462348]  ___sys_sendmsg+0x81/0xc0
>>>  [  523.462350]  ? netlink_seq_start+0x70/0x70
>>>  [  523.462352]  ? __dentry_kill+0x13a/0x180
>>>  [  523.462354]  ? __fput+0xff/0x250
>>>  [  523.462356]  __sys_sendmsg+0x49/0x80
>>>  [  523.462358]  do_syscall_64+0x3b/0x90
>>>  [  523.462361]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>>>  [  523.462364] RIP: 0033:0x7f24552aa337
>>>  [  523.462365] Code: 0e 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
>>>  [  523.462366] RSP: 002b:00007fff7f05a838 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>>>  [  523.462368] RAX: ffffffffffffffda RBX: 000000006245bf91 RCX: 00007f24552aa337
>>>  [  523.462368] RDX: 0000000000000000 RSI: 00007fff7f05a8a0 RDI: 0000000000000003
>>>  [  523.462369] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
>>>  [  523.462370] R10: 0000000000000008 R11: 0000000000000246 R12: 0000000000000001
>>>  [  523.462370] R13: 00007fff7f05ce08 R14: 0000000000000000 R15: 000055e5dfdd1040
>>>  [  523.462373]  </TASK>
>>>  [  523.462374] ---[ end trace ba537bc16f6bf4ed ]---
>>> 
>>> [2] https://github.com/FRRouting/frr/issues/6412
>>> 
>>> Fixes: 4c7e8084fd46 ("ipv4: Plumb support for nexthop object in a fib_info")
>>> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.org>
>>> ---
>>>  net/ipv4/fib_semantics.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>>> index cc8e84ef2ae4..ccb62038f6a4 100644
>>> --- a/net/ipv4/fib_semantics.c
>>> +++ b/net/ipv4/fib_semantics.c
>>> @@ -889,8 +889,13 @@ int fib_nh_match(struct net *net, struct fib_config *cfg, struct fib_info *fi,
>>>  	}
>>>  
>>>  	if (cfg->fc_oif || cfg->fc_gw_family) {
>>> -		struct fib_nh *nh = fib_info_nh(fi, 0);
>>> +		struct fib_nh *nh;
>>> +
>>> +		/* cannot match on nexthop object attributes */
>>> +		if (fi->nh)
>>> +			return 1;
>>>  
>>> +		nh = fib_info_nh(fi, 0);
>>>  		if (cfg->fc_encap) {
>>>  			if (fib_encap_match(net, cfg->fc_encap_type,
>>>  					    cfg->fc_encap, nh, cfg, extack))
>>
>>I think the right fix is something like this:
>>diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>>index cc8e84ef2ae4..c70775f5e155 100644
>>--- a/net/ipv4/fib_semantics.c
>>+++ b/net/ipv4/fib_semantics.c
>>@@ -886,6 +886,8 @@ int fib_nh_match(struct net *net, struct fib_config
>>*cfg, struct fib_info *fi,
>>                if (fi->nh && cfg->fc_nh_id == fi->nh->id)
>>                        return 0;
>>                return 1;
>>+       } else if (fi->nh) {
>>+               return 1;
>>        }
>>
>>ie., if the cfg has a nexthop id it needs to match fib_info.
>>if the cfg does not have a nexthop id, but fib_info does then it is not
>>a match
>
>
>Right, that is also correct and I was tempted to cut it all short in that case
> but seemed riskier so I went with the more narrow fix for that specific case.
>Anyway, I'll respin with that change.
>
>Cheers,
> Nik
>

Actually I just remembered another reason - ip route del default
should work regardless of specifying nhid or not. I can't test right now,
but I suspect that may break if the nh match code is invoked with that change.

I'll verify it once I'm back in front of a pc.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ