| Message ID | 733d454d3c36e99b55de5374c7664364975b171d.1493313626.git.pabeni@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Apr 27, 2017 at 07:29:34PM +0200, Paolo Abeni wrote: > On slave list updates, the bonding driver computes its hard_header_len > as the maximum of all enslaved devices's hard_header_len. > If the slave list is empty, e.g. on last enslaved device removal, > ETH_HLEN is used. > > Since the bonding header_ops are set only when the first enslaved > device is attached, the above can lead to header_ops->create() > being called with the wrong skb headroom in place. > > If bond0 is configured on top of ipoib devices, with the > following commands: > > ifup bond0 > for slave in $BOND_SLAVES_LIST; do > ip link set dev $slave nomaster > done > ping -c 1 <ip on bond0 subnet> > > we will obtain a skb_under_panic() with a similar call trace: > skb_push+0x3d/0x40 > push_pseudo_header+0x17/0x30 [ib_ipoib] > ipoib_hard_header+0x4e/0x80 [ib_ipoib] > arp_create+0x12f/0x220 > arp_send_dst.part.19+0x28/0x50 > arp_solicit+0x115/0x290 > neigh_probe+0x4d/0x70 > __neigh_event_send+0xa7/0x230 > neigh_resolve_output+0x12e/0x1c0 > ip_finish_output2+0x14b/0x390 > ip_finish_output+0x136/0x1e0 > ip_output+0x76/0xe0 > ip_local_out+0x35/0x40 > ip_send_skb+0x19/0x40 > ip_push_pending_frames+0x33/0x40 > raw_sendmsg+0x7d3/0xb50 > inet_sendmsg+0x31/0xb0 > sock_sendmsg+0x38/0x50 > SYSC_sendto+0x102/0x190 > SyS_sendto+0xe/0x10 > do_syscall_64+0x67/0x180 > entry_SYSCALL64_slow_path+0x25/0x25 > > This change addresses the issue avoiding updating the bonding device > hard_header_len when the slaves list become empty, forbidding to > shrink it below the value used by header_ops->create(). > > The bug is there since commit 54ef31371407 ("[PATCH] bonding: Handle large > hard_header_len") but the panic can be triggered only since > commit fc791b633515 ("IB/ipoib: move back IB LL address into the hard > header"). > > Reported-by: Norbert P <noe@physik.uzh.ch> > Fixes: 54ef31371407 ("[PATCH] bonding: Handle large hard_header_len") > Fixes: fc791b633515 ("IB/ipoib: move back IB LL address into the hard header") > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- Thanks Paolo. > drivers/net/bonding/bond_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 8a4ba8b..34481c9 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1104,11 +1104,11 @@ static void bond_compute_features(struct bonding *bond) > gso_max_size = min(gso_max_size, slave->dev->gso_max_size); > gso_max_segs = min(gso_max_segs, slave->dev->gso_max_segs); > } > + bond_dev->hard_header_len = max_hard_header_len; > > done: > bond_dev->vlan_features = vlan_features; > bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL; > - bond_dev->hard_header_len = max_hard_header_len; > bond_dev->gso_max_segs = gso_max_segs; > netif_set_gso_max_size(bond_dev, gso_max_size); > > -- > 2.9.3 >
Paolo Abeni <pabeni@redhat.com> wrote: >On slave list updates, the bonding driver computes its hard_header_len >as the maximum of all enslaved devices's hard_header_len. >If the slave list is empty, e.g. on last enslaved device removal, >ETH_HLEN is used. > >Since the bonding header_ops are set only when the first enslaved >device is attached, the above can lead to header_ops->create() >being called with the wrong skb headroom in place. > >If bond0 is configured on top of ipoib devices, with the >following commands: > >ifup bond0 >for slave in $BOND_SLAVES_LIST; do > ip link set dev $slave nomaster >done >ping -c 1 <ip on bond0 subnet> > >we will obtain a skb_under_panic() with a similar call trace: > skb_push+0x3d/0x40 > push_pseudo_header+0x17/0x30 [ib_ipoib] > ipoib_hard_header+0x4e/0x80 [ib_ipoib] > arp_create+0x12f/0x220 > arp_send_dst.part.19+0x28/0x50 > arp_solicit+0x115/0x290 > neigh_probe+0x4d/0x70 > __neigh_event_send+0xa7/0x230 > neigh_resolve_output+0x12e/0x1c0 > ip_finish_output2+0x14b/0x390 > ip_finish_output+0x136/0x1e0 > ip_output+0x76/0xe0 > ip_local_out+0x35/0x40 > ip_send_skb+0x19/0x40 > ip_push_pending_frames+0x33/0x40 > raw_sendmsg+0x7d3/0xb50 > inet_sendmsg+0x31/0xb0 > sock_sendmsg+0x38/0x50 > SYSC_sendto+0x102/0x190 > SyS_sendto+0xe/0x10 > do_syscall_64+0x67/0x180 > entry_SYSCALL64_slow_path+0x25/0x25 > >This change addresses the issue avoiding updating the bonding device >hard_header_len when the slaves list become empty, forbidding to >shrink it below the value used by header_ops->create(). > >The bug is there since commit 54ef31371407 ("[PATCH] bonding: Handle large >hard_header_len") but the panic can be triggered only since >commit fc791b633515 ("IB/ipoib: move back IB LL address into the hard >header"). > >Reported-by: Norbert P <noe@physik.uzh.ch> >Fixes: 54ef31371407 ("[PATCH] bonding: Handle large hard_header_len") >Fixes: fc791b633515 ("IB/ipoib: move back IB LL address into the hard header") >Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >Signed-off-by: Paolo Abeni <pabeni@redhat.com> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com> > drivers/net/bonding/bond_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 8a4ba8b..34481c9 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1104,11 +1104,11 @@ static void bond_compute_features(struct bonding *bond) > gso_max_size = min(gso_max_size, slave->dev->gso_max_size); > gso_max_segs = min(gso_max_segs, slave->dev->gso_max_segs); > } >+ bond_dev->hard_header_len = max_hard_header_len; > > done: > bond_dev->vlan_features = vlan_features; > bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL; >- bond_dev->hard_header_len = max_hard_header_len; > bond_dev->gso_max_segs = gso_max_segs; > netif_set_gso_max_size(bond_dev, gso_max_size); > >-- >2.9.3 >
From: Paolo Abeni <pabeni@redhat.com> Date: Thu, 27 Apr 2017 19:29:34 +0200 > On slave list updates, the bonding driver computes its hard_header_len > as the maximum of all enslaved devices's hard_header_len. > If the slave list is empty, e.g. on last enslaved device removal, > ETH_HLEN is used. > > Since the bonding header_ops are set only when the first enslaved > device is attached, the above can lead to header_ops->create() > being called with the wrong skb headroom in place. > > If bond0 is configured on top of ipoib devices, with the > following commands: > > ifup bond0 > for slave in $BOND_SLAVES_LIST; do > ip link set dev $slave nomaster > done > ping -c 1 <ip on bond0 subnet> > > we will obtain a skb_under_panic() with a similar call trace: ... > This change addresses the issue avoiding updating the bonding device > hard_header_len when the slaves list become empty, forbidding to > shrink it below the value used by header_ops->create(). > > The bug is there since commit 54ef31371407 ("[PATCH] bonding: Handle large > hard_header_len") but the panic can be triggered only since > commit fc791b633515 ("IB/ipoib: move back IB LL address into the hard > header"). > > Reported-by: Norbert P <noe@physik.uzh.ch> > Fixes: 54ef31371407 ("[PATCH] bonding: Handle large hard_header_len") > Fixes: fc791b633515 ("IB/ipoib: move back IB LL address into the hard header") > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > Signed-off-by: Paolo Abeni <pabeni@redhat.com> Applied, thanks.
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 8a4ba8b..34481c9 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1104,11 +1104,11 @@ static void bond_compute_features(struct bonding *bond) gso_max_size = min(gso_max_size, slave->dev->gso_max_size); gso_max_segs = min(gso_max_segs, slave->dev->gso_max_segs); } + bond_dev->hard_header_len = max_hard_header_len; done: bond_dev->vlan_features = vlan_features; bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL; - bond_dev->hard_header_len = max_hard_header_len; bond_dev->gso_max_segs = gso_max_segs; netif_set_gso_max_size(bond_dev, gso_max_size);
On slave list updates, the bonding driver computes its hard_header_len as the maximum of all enslaved devices's hard_header_len. If the slave list is empty, e.g. on last enslaved device removal, ETH_HLEN is used. Since the bonding header_ops are set only when the first enslaved device is attached, the above can lead to header_ops->create() being called with the wrong skb headroom in place. If bond0 is configured on top of ipoib devices, with the following commands: ifup bond0 for slave in $BOND_SLAVES_LIST; do ip link set dev $slave nomaster done ping -c 1 <ip on bond0 subnet> we will obtain a skb_under_panic() with a similar call trace: skb_push+0x3d/0x40 push_pseudo_header+0x17/0x30 [ib_ipoib] ipoib_hard_header+0x4e/0x80 [ib_ipoib] arp_create+0x12f/0x220 arp_send_dst.part.19+0x28/0x50 arp_solicit+0x115/0x290 neigh_probe+0x4d/0x70 __neigh_event_send+0xa7/0x230 neigh_resolve_output+0x12e/0x1c0 ip_finish_output2+0x14b/0x390 ip_finish_output+0x136/0x1e0 ip_output+0x76/0xe0 ip_local_out+0x35/0x40 ip_send_skb+0x19/0x40 ip_push_pending_frames+0x33/0x40 raw_sendmsg+0x7d3/0xb50 inet_sendmsg+0x31/0xb0 sock_sendmsg+0x38/0x50 SYSC_sendto+0x102/0x190 SyS_sendto+0xe/0x10 do_syscall_64+0x67/0x180 entry_SYSCALL64_slow_path+0x25/0x25 This change addresses the issue avoiding updating the bonding device hard_header_len when the slaves list become empty, forbidding to shrink it below the value used by header_ops->create(). The bug is there since commit 54ef31371407 ("[PATCH] bonding: Handle large hard_header_len") but the panic can be triggered only since commit fc791b633515 ("IB/ipoib: move back IB LL address into the hard header"). Reported-by: Norbert P <noe@physik.uzh.ch> Fixes: 54ef31371407 ("[PATCH] bonding: Handle large hard_header_len") Fixes: fc791b633515 ("IB/ipoib: move back IB LL address into the hard header") Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- drivers/net/bonding/bond_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)