Subject: Re: [PATCH net v2] bonding: fix div by zero while enslaving and transmitting

From: Ding Tianhong <[email-masked]>
Date: Wed, 17 Sep 2014 14:15:07 +0800

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 <[email-masked]>
> CC: Andy Gospodarek <[email-masked]>
> CC: Jay Vosburgh <[email-masked]>
> CC: Veaceslav Falico <[email-masked]>
> Fixes: 278b208375 ("bonding: initial RCU conversion")
> Signed-off-by: Nikolay Aleksandrov <[email-masked]>
> ---
> 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;
>  }
>