| 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
| ||
|
Message-ID: <541926EB.4010809@huawei.com>
Date: Wed, 17 Sep 2014 14:15:07 +0800
From: Ding Tianhong <dingtianhong@...wei.com>
To: Nikolay Aleksandrov <nikolay@...hat.com>, <netdev@...r.kernel.org>
CC: Eric Dumazet <eric.dumazet@...il.com>,
Andy Gospodarek <andy@...yhouse.net>,
Jay Vosburgh <j.vosburgh@...il.com>,
Veaceslav Falico <vfalico@...il.com>
Subject: Re: [PATCH net v2] bonding: fix div by zero while enslaving and transmitting
On 2014/9/12 23:38, Nikolay Aleksandrov wrote:
> The problem is that the slave is first linked and slave_cnt is
> incremented afterwards leading to a div by zero in the modes that use it
> as a modulus. What happens is that in bond_start_xmit()
> bond_has_slaves() is used to evaluate further transmission and it becomes
> true after the slave is linked in, but when slave_cnt is used in the xmit
> path it is still 0, so fetch it once and transmit based on that. Since
> it is used only in round-robin and XOR modes, the fix is only for them.
> Thanks to Eric Dumazet for pointing out the fault in my first try to fix
> this.
>
Hi, I think no need to add more checks in the xmit fast path, why not add a barrier to make
sure the slave_cnt inc to 1 before access it.
+ /* Increment slave_cnt before linking in the slave so we won't end up in
+ * bond_start_xmit with bond_has_slaves() true and slave_cnt == 0.
+ */
+ bond->slave_cnt++;
+ wmb();
I think it looks more efficiency, sorry for reply so late.
Regards
Ding
> Call trace (took it out of net-next kernel, but it's the same with net):
> [46934.330038] divide error: 0000 [#1] SMP
> [46934.330041] Modules linked in: bonding(O) 9p fscache
> snd_hda_codec_generic crct10dif_pclmul
> [46934.330041] bond0: Enslaving eth1 as an active interface with an up
> link
> [46934.330051] ppdev joydev crc32_pclmul crc32c_intel 9pnet_virtio
> ghash_clmulni_intel snd_hda_intel 9pnet snd_hda_controller parport_pc
> serio_raw pcspkr snd_hda_codec parport virtio_balloon virtio_console
> snd_hwdep snd_pcm pvpanic i2c_piix4 snd_timer i2ccore snd soundcore
> virtio_blk virtio_net virtio_pci virtio_ring virtio ata_generic
> pata_acpi floppy [last unloaded: bonding]
> [46934.330053] CPU: 1 PID: 3382 Comm: ping Tainted: G O
> 3.17.0-rc4+ #27
> [46934.330053] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [46934.330054] task: ffff88005aebf2c0 ti: ffff88005b728000 task.ti:
> ffff88005b728000
> [46934.330059] RIP: 0010:[<ffffffffa0198c33>] [<ffffffffa0198c33>]
> bond_start_xmit+0x1c3/0x450 [bonding]
> [46934.330060] RSP: 0018:ffff88005b72b7f8 EFLAGS: 00010246
> [46934.330060] RAX: 0000000000000679 RBX: ffff88004b077000 RCX:
> 000000000000002a
> [46934.330061] RDX: 0000000000000000 RSI: ffff88004b3f0500 RDI:
> ffff88004b077940
> [46934.330061] RBP: ffff88005b72b830 R08: 00000000000000c0 R09:
> ffff88004a83e000
> [46934.330062] R10: 000000000000ffff R11: ffff88004b1f12c0 R12:
> ffff88004b3f0500
> [46934.330062] R13: ffff88004b3f0500 R14: 000000000000002a R15:
> ffff88004b077940
> [46934.330063] FS: 00007fbd91a4c740(0000) GS:ffff88005f080000(0000)
> knlGS:0000000000000000
> [46934.330064] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [46934.330064] CR2: 00007f803a8bb000 CR3: 000000004b2c9000 CR4:
> 00000000000406e0
> [46934.330069] Stack:
> [46934.330071] ffffffff811e6169 00000000e772fa05 ffff88004b077000
> ffff88004b3f0500
> [46934.330072] ffffffff81d17d18 000000000000002a 0000000000000000
> ffff88005b72b8a0
> [46934.330073] ffffffff81620108 ffffffff8161fe0e ffff88005b72b8c4
> ffff88005b302000
> [46934.330073] Call Trace:
> [46934.330077] [<ffffffff811e6169>] ?
> __kmalloc_node_track_caller+0x119/0x300
> [46934.330084] [<ffffffff81620108>] dev_hard_start_xmit+0x188/0x410
> [46934.330086] [<ffffffff8161fe0e>] ? harmonize_features+0x2e/0x90
> [46934.330088] [<ffffffff81620b06>] __dev_queue_xmit+0x456/0x590
> [46934.330089] [<ffffffff81620c50>] dev_queue_xmit+0x10/0x20
> [46934.330090] [<ffffffff8168f022>] arp_xmit+0x22/0x60
> [46934.330091] [<ffffffff8168f090>] arp_send.part.16+0x30/0x40
> [46934.330092] [<ffffffff8168f1e5>] arp_solicit+0x115/0x2b0
> [46934.330094] [<ffffffff8160b5d7>] ? copy_skb_header+0x17/0xa0
> [46934.330096] [<ffffffff8162875a>] neigh_probe+0x4a/0x70
> [46934.330097] [<ffffffff8162979c>] __neigh_event_send+0xac/0x230
> [46934.330098] [<ffffffff8162a00b>] neigh_resolve_output+0x13b/0x220
> [46934.330100] [<ffffffff8165f120>] ? ip_forward_options+0x1c0/0x1c0
> [46934.330101] [<ffffffff81660478>] ip_finish_output+0x1f8/0x860
> [46934.330102] [<ffffffff81661f08>] ip_output+0x58/0x90
> [46934.330103] [<ffffffff81661602>] ? __ip_local_out+0xa2/0xb0
> [46934.330104] [<ffffffff81661640>] ip_local_out_sk+0x30/0x40
> [46934.330105] [<ffffffff81662a66>] ip_send_skb+0x16/0x50
> [46934.330106] [<ffffffff81662ad3>] ip_push_pending_frames+0x33/0x40
> [46934.330107] [<ffffffff8168854c>] raw_sendmsg+0x88c/0xa30
> [46934.330110] [<ffffffff81612b31>] ? skb_recv_datagram+0x41/0x60
> [46934.330111] [<ffffffff816875a9>] ? raw_recvmsg+0xa9/0x1f0
> [46934.330113] [<ffffffff816978d4>] inet_sendmsg+0x74/0xc0
> [46934.330114] [<ffffffff81697a9b>] ? inet_recvmsg+0x8b/0xb0
> [46934.330115] bond0: Adding slave eth2
> [46934.330116] [<ffffffff8160357c>] sock_sendmsg+0x9c/0xe0
> [46934.330118] [<ffffffff81603248>] ?
> move_addr_to_kernel.part.20+0x28/0x80
> [46934.330121] [<ffffffff811b4477>] ? might_fault+0x47/0x50
> [46934.330122] [<ffffffff816039b9>] ___sys_sendmsg+0x3a9/0x3c0
> [46934.330125] [<ffffffff8144a14a>] ? n_tty_write+0x3aa/0x530
> [46934.330127] [<ffffffff810d1ae4>] ? __wake_up+0x44/0x50
> [46934.330129] [<ffffffff81242b38>] ? fsnotify+0x238/0x310
> [46934.330130] [<ffffffff816048a1>] __sys_sendmsg+0x51/0x90
> [46934.330131] [<ffffffff816048f2>] SyS_sendmsg+0x12/0x20
> [46934.330134] [<ffffffff81738b29>] system_call_fastpath+0x16/0x1b
> [46934.330144] Code: 48 8b 10 4c 89 ee 4c 89 ff e8 aa bc ff ff 31 c0 e9
> 1a ff ff ff 0f 1f 00 4c 89 ee 4c 89 ff e8 65 fb ff ff 31 d2 4c 89 ee 4c
> 89 ff <f7> b3 64 09 00 00 e8 02 bd ff ff 31 c0 e9 f2 fe ff ff 0f 1f 00
> [46934.330146] RIP [<ffffffffa0198c33>] bond_start_xmit+0x1c3/0x450
> [bonding]
> [46934.330146] RSP <ffff88005b72b7f8>
>
> CC: Eric Dumazet <eric.dumazet@...il.com>
> CC: Andy Gospodarek <andy@...yhouse.net>
> CC: Jay Vosburgh <j.vosburgh@...il.com>
> CC: Veaceslav Falico <vfalico@...il.com>
> Fixes: 278b208375 ("bonding: initial RCU conversion")
> Signed-off-by: Nikolay Aleksandrov <nikolay@...hat.com>
> ---
> v2: Based on Eric's feedback change the fix to fetch the value once in a
> local variable in the affected modes and to act based on that.
>
> I believe we don't need to move the increment now as it doesn't really
> matter if it'll be before the wmb or after.
>
> drivers/net/bonding/bond_main.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 57912ee231cb..798ae69fb63c 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3659,8 +3659,14 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
> else
> bond_xmit_slave_id(bond, skb, 0);
> } else {
> - slave_id = bond_rr_gen_slave_id(bond);
> - bond_xmit_slave_id(bond, skb, slave_id % bond->slave_cnt);
> + int slave_cnt = ACCESS_ONCE(bond->slave_cnt);
> +
> + if (likely(slave_cnt)) {
> + slave_id = bond_rr_gen_slave_id(bond);
> + bond_xmit_slave_id(bond, skb, slave_id % slave_cnt);
> + } else {
> + dev_kfree_skb_any(skb);
> + }
> }
>
> return NETDEV_TX_OK;
> @@ -3691,8 +3697,13 @@ static int bond_xmit_activebackup(struct sk_buff *skb, struct net_device *bond_d
> static int bond_xmit_xor(struct sk_buff *skb, struct net_device *bond_dev)
> {
> struct bonding *bond = netdev_priv(bond_dev);
> + int slave_cnt = ACCESS_ONCE(bond->slave_cnt);
>
> - bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) % bond->slave_cnt);
> + if (likely(slave_cnt))
> + bond_xmit_slave_id(bond, skb,
> + bond_xmit_hash(bond, skb) % slave_cnt);
> + else
> + dev_kfree_skb_any(skb);
>
> return NETDEV_TX_OK;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists