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
| ||
|
Date: Wed, 14 Apr 2010 16:11:02 +0800 From: Cong Wang <amwang@...hat.com> To: Jay Vosburgh <fubar@...ibm.com> CC: Stephen Hemminger <shemminger@...ux-foundation.org>, Eric Dumazet <eric.dumazet@...il.com>, Neil Horman <nhorman@...driver.com>, netdev@...r.kernel.org, Andy Gospodarek <gospo@...hat.com>, bridge@...ts.linux-foundation.org, linux-kernel@...r.kernel.org, bonding-devel@...ts.sourceforge.net, Jeff Moyer <jmoyer@...hat.com>, Matt Mackall <mpm@...enic.com>, David Miller <davem@...emloft.net> Subject: Re: [Bonding-devel] [v3 Patch 2/3] bridge: make bridge support netpoll Jay Vosburgh wrote: > Cong Wang <amwang@...hat.com> wrote: > >> Stephen Hemminger wrote: >>> On Mon, 12 Apr 2010 12:38:57 +0200 >>> Eric Dumazet <eric.dumazet@...il.com> wrote: >>> >>>> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit : >>>>> Stephen Hemminger wrote: >>>>>> There is no protection on dev->priv_flags for SMP access. >>>>>> It would better bit value in dev->state if you are using it as control flag. >>>>>> >>>>>> Then you could use >>>>>> if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state))) >>>>>> netpoll_send_skb(...) >>>>>> >>>>>> >>>>> Hmm, I think we can't use ->state here, it is not for this kind of purpose, >>>>> according to its comments. >>>>> >>>>> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using >>>>> &, | to set or clear the flags. So there must be some other things preventing >>>>> the race... >>>> Yes, its RTNL that protects priv_flags changes, hopefully... >>>> >>> The patch was not protecting priv_flags with RTNL. >>> For example.. >>> >>> >>> @@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp >>> tries > 0; --tries) { >>> if (__netif_tx_trylock(txq)) { >>> if (!netif_tx_queue_stopped(txq)) { >>> + dev->priv_flags |= IFF_IN_NETPOLL; >>> status = ops->ndo_start_xmit(skb, dev); >>> + dev->priv_flags &= ~IFF_IN_NETPOLL; >>> if (status == NETDEV_TX_OK) >>> txq_trans_update(txq); >> Hmm, but I checked the bonding case (IFF_BONDING), it doesn't >> hold rtnl_lock. Strange. > > I looked, and there are a couple of cases in bonding that don't > have RTNL for adjusting priv_flags (in bond_ab_arp_probe when no slaves > are up, and a couple of cases in 802.3ad). I think the solution there > is to move bonding away from priv_flags for some of this (e.g., convert > bonding to use a frame hook like bridge and macvlan, and greatly > simplify skb_bond_should_drop), but that's a separate topic. > > The majority of the cases, however, do hold RTNL. Bonding > generally doesn't have to acquire RTNL itself, since whatever called > into bonding is holding it already. For example, the slave add and > remove paths (bond_enslave, bond_release) are called either via sysfs or > ioctl, both of which acquire RTNL. All of the set and clear operations > for IFF_BONDING fall into this category; look at bonding_store_slaves > for an example. > > Bonding does acquire RTNL itself when performing failovers, > e.g., bond_mii_monitor holds RTNL prior to calling bond_miimon_commit, > which will change priv_flags. > Thanks a lot for your reply! You are right, I missed something. Hmm, for bonding, RTNL lock is necessary because there are sysfs interface and ioctl interface to change its configuration. -- 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