| Message ID | 1392648088-21336-2-git-send-email-vfalico@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2014/2/17 22:41, Veaceslav Falico wrote: > We're always called with rcu_read_lock() held (bond_arp_rcv() is only > called from bond_handle_frame(), which is rx_handler and always called > under rcu from __netif_receive_skb_core() ). > > The slave active/passive and/or bonding params can change in-flight, however > we don't really care about that - we only modify the last time packet was > received, which is harmless. > > CC: Jay Vosburgh <fubar@us.ibm.com> > CC: Andy Gospodarek <andy@greyhouse.net> > Signed-off-by: Veaceslav Falico <vfalico@redhat.com> > --- > drivers/net/bonding/bond_main.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 3bce855..3c50bec 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -2260,8 +2260,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, > if (skb->protocol != __cpu_to_be16(ETH_P_ARP)) > return RX_HANDLER_ANOTHER; > > - read_lock(&bond->lock); > - > if (!slave_do_arp_validate(bond, slave)) > goto out_unlock; > > @@ -2318,7 +2316,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, > bond_validate_arp(bond, slave, tip, sip); > > out_unlock: > - read_unlock(&bond->lock); > if (arp != (struct arphdr *)skb->data) > kfree(arp); > return RX_HANDLER_ANOTHER; > I think it is not enough, you should add rcu_dereference for bond->curr_active_slave, it may be changed during the recv processing. Regards Ding -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 18, 2014 at 12:02:41PM +0800, Ding Tianhong wrote: >On 2014/2/17 22:41, Veaceslav Falico wrote: >> We're always called with rcu_read_lock() held (bond_arp_rcv() is only >> called from bond_handle_frame(), which is rx_handler and always called >> under rcu from __netif_receive_skb_core() ). >> >> The slave active/passive and/or bonding params can change in-flight, however >> we don't really care about that - we only modify the last time packet was >> received, which is harmless. >> >> CC: Jay Vosburgh <fubar@us.ibm.com> >> CC: Andy Gospodarek <andy@greyhouse.net> >> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> >> --- >> drivers/net/bonding/bond_main.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 3bce855..3c50bec 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -2260,8 +2260,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, >> if (skb->protocol != __cpu_to_be16(ETH_P_ARP)) >> return RX_HANDLER_ANOTHER; >> >> - read_lock(&bond->lock); >> - >> if (!slave_do_arp_validate(bond, slave)) >> goto out_unlock; >> >> @@ -2318,7 +2316,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, >> bond_validate_arp(bond, slave, tip, sip); >> >> out_unlock: >> - read_unlock(&bond->lock); >> if (arp != (struct arphdr *)skb->data) >> kfree(arp); >> return RX_HANDLER_ANOTHER; >> > >I think it is not enough, you should add rcu_dereference for bond->curr_active_slave, it may be changed during >the recv processing. bond->lock has absolutely nothing to du with bond->curr_active_slave . > >Regards >Ding > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/2/18 14:12, Veaceslav Falico wrote: > On Tue, Feb 18, 2014 at 12:02:41PM +0800, Ding Tianhong wrote: >> On 2014/2/17 22:41, Veaceslav Falico wrote: >>> We're always called with rcu_read_lock() held (bond_arp_rcv() is only >>> called from bond_handle_frame(), which is rx_handler and always called >>> under rcu from __netif_receive_skb_core() ). >>> >>> The slave active/passive and/or bonding params can change in-flight, however >>> we don't really care about that - we only modify the last time packet was >>> received, which is harmless. >>> >>> CC: Jay Vosburgh <fubar@us.ibm.com> >>> CC: Andy Gospodarek <andy@greyhouse.net> >>> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> >>> --- >>> drivers/net/bonding/bond_main.c | 3 --- >>> 1 file changed, 3 deletions(-) >>> >>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >>> index 3bce855..3c50bec 100644 >>> --- a/drivers/net/bonding/bond_main.c >>> +++ b/drivers/net/bonding/bond_main.c >>> @@ -2260,8 +2260,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, >>> if (skb->protocol != __cpu_to_be16(ETH_P_ARP)) >>> return RX_HANDLER_ANOTHER; >>> >>> - read_lock(&bond->lock); >>> - >>> if (!slave_do_arp_validate(bond, slave)) >>> goto out_unlock; >>> >>> @@ -2318,7 +2316,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, >>> bond_validate_arp(bond, slave, tip, sip); >>> >>> out_unlock: >>> - read_unlock(&bond->lock); >>> if (arp != (struct arphdr *)skb->data) >>> kfree(arp); >>> return RX_HANDLER_ANOTHER; >>> >> >> I think it is not enough, you should add rcu_dereference for bond->curr_active_slave, it may be changed during >> the recv processing. > > bond->lock has absolutely nothing to du with bond->curr_active_slave . > Yep, this problem is introduced by commit aeea64ac7， there is no way to protect the curr_active_slave, so I think you could fix it in this patch together. else if (bond->curr_active_slave && time_after(slave_last_rx(bond, bond->curr_active_slave), bond->curr_active_slave->jiffies)) >> >> Regards >> Ding >> >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 18, 2014 at 03:07:46PM +0800, Ding Tianhong wrote: >On 2014/2/18 14:12, Veaceslav Falico wrote: >> On Tue, Feb 18, 2014 at 12:02:41PM +0800, Ding Tianhong wrote: >>> On 2014/2/17 22:41, Veaceslav Falico wrote: ...snip... >>> I think it is not enough, you should add rcu_dereference for bond->curr_active_slave, it may be changed during >>> the recv processing. >> >> bond->lock has absolutely nothing to du with bond->curr_active_slave . >> >Yep, this problem is introduced by commit aeea64ac7， there is no way to protect the curr_active_slave, so >I think you could fix it in this patch together. > > else if (bond->curr_active_slave && > time_after(slave_last_rx(bond, bond->curr_active_slave), > bond->curr_active_slave->jiffies)) It's not related to this patchset, but yeah, I'll send a fix afterwards. >>> >>> Regards >>> Ding >>> >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 3bce855..3c50bec 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2260,8 +2260,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, if (skb->protocol != __cpu_to_be16(ETH_P_ARP)) return RX_HANDLER_ANOTHER; - read_lock(&bond->lock); - if (!slave_do_arp_validate(bond, slave)) goto out_unlock; @@ -2318,7 +2316,6 @@ int bond_arp_rcv(const struct sk_buff *skb, struct bonding *bond, bond_validate_arp(bond, slave, tip, sip); out_unlock: - read_unlock(&bond->lock); if (arp != (struct arphdr *)skb->data) kfree(arp); return RX_HANDLER_ANOTHER;
We're always called with rcu_read_lock() held (bond_arp_rcv() is only called from bond_handle_frame(), which is rx_handler and always called under rcu from __netif_receive_skb_core() ). The slave active/passive and/or bonding params can change in-flight, however we don't really care about that - we only modify the last time packet was received, which is harmless. CC: Jay Vosburgh <fubar@us.ibm.com> CC: Andy Gospodarek <andy@greyhouse.net> Signed-off-by: Veaceslav Falico <vfalico@redhat.com> --- drivers/net/bonding/bond_main.c | 3 --- 1 file changed, 3 deletions(-)