From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ding Tianhong Subject: Re: [PATCH net v2] bonding: fix div by zero while enslaving and transmitting Date: Wed, 17 Sep 2014 14:15:07 +0800 Message-ID: <541926EB.4010809@huawei.com> References: <5413097B.9080802@redhat.com> <1410536298-8022-1-git-send-email-nikolay@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Eric Dumazet , Andy Gospodarek , Jay Vosburgh , Veaceslav Falico To: Nikolay Aleksandrov , Return-path: Received: from szxga03-in.huawei.com ([119.145.14.66]:37587 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751327AbaIQGPf (ORCPT ); Wed, 17 Sep 2014 02:15:35 -0400 In-Reply-To: <1410536298-8022-1-git-send-email-nikolay@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: 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:[] [] > 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] [] ? > __kmalloc_node_track_caller+0x119/0x300 > [46934.330084] [] dev_hard_start_xmit+0x188/0x410 > [46934.330086] [] ? harmonize_features+0x2e/0x90 > [46934.330088] [] __dev_queue_xmit+0x456/0x590 > [46934.330089] [] dev_queue_xmit+0x10/0x20 > [46934.330090] [] arp_xmit+0x22/0x60 > [46934.330091] [] arp_send.part.16+0x30/0x40 > [46934.330092] [] arp_solicit+0x115/0x2b0 > [46934.330094] [] ? copy_skb_header+0x17/0xa0 > [46934.330096] [] neigh_probe+0x4a/0x70 > [46934.330097] [] __neigh_event_send+0xac/0x230 > [46934.330098] [] neigh_resolve_output+0x13b/0x220 > [46934.330100] [] ? ip_forward_options+0x1c0/0x1c0 > [46934.330101] [] ip_finish_output+0x1f8/0x860 > [46934.330102] [] ip_output+0x58/0x90 > [46934.330103] [] ? __ip_local_out+0xa2/0xb0 > [46934.330104] [] ip_local_out_sk+0x30/0x40 > [46934.330105] [] ip_send_skb+0x16/0x50 > [46934.330106] [] ip_push_pending_frames+0x33/0x40 > [46934.330107] [] raw_sendmsg+0x88c/0xa30 > [46934.330110] [] ? skb_recv_datagram+0x41/0x60 > [46934.330111] [] ? raw_recvmsg+0xa9/0x1f0 > [46934.330113] [] inet_sendmsg+0x74/0xc0 > [46934.330114] [] ? inet_recvmsg+0x8b/0xb0 > [46934.330115] bond0: Adding slave eth2 > [46934.330116] [] sock_sendmsg+0x9c/0xe0 > [46934.330118] [] ? > move_addr_to_kernel.part.20+0x28/0x80 > [46934.330121] [] ? might_fault+0x47/0x50 > [46934.330122] [] ___sys_sendmsg+0x3a9/0x3c0 > [46934.330125] [] ? n_tty_write+0x3aa/0x530 > [46934.330127] [] ? __wake_up+0x44/0x50 > [46934.330129] [] ? fsnotify+0x238/0x310 > [46934.330130] [] __sys_sendmsg+0x51/0x90 > [46934.330131] [] SyS_sendmsg+0x12/0x20 > [46934.330134] [] 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 b3 64 09 00 00 e8 02 bd ff ff 31 c0 e9 f2 fe ff ff 0f 1f 00 > [46934.330146] RIP [] bond_start_xmit+0x1c3/0x450 > [bonding] > [46934.330146] RSP > > CC: Eric Dumazet > CC: Andy Gospodarek > CC: Jay Vosburgh > CC: Veaceslav Falico > Fixes: 278b208375 ("bonding: initial RCU conversion") > Signed-off-by: Nikolay Aleksandrov > --- > 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; > } >