Message ID | 20091021130301.GA4762@midget.suse.cz |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Jiri Bohac <jbohac@suse.cz> wrote: >In mii monitor mode, bond_check_dev_link() calls the the ioctl >handler of slave devices. It stores the ndo_do_ioctl function >pointer to a static (!) ioctl variable and later uses it to call the >handler with the IOCTL macro. > >If another thread executes bond_check_dev_link() at the same time >(even with a different bond, which none of the locks prevent), a >race condition occurs. If the two racing slaves have different >drivers, this may result in one driver's ioctl handler being >called with a pointer to a net_device controlled with a different >driver, resulting in unpredictable breakage. > >Unless I am overlooking something, the "static" must be a >copy'n'paste error (?). Heh, I was curious, so I looked it up; this bit was added as-is in September 2000, when the original "miimon" link monitoring code was added. It's interesting that nobody hit this bug back in the days before netif_carrier; I know I ran a lot of mixed slave environments. >Signed-off-by: Jiri Bohac <jbohac@suse.cz> Anyway, the static is obviously wrong, even without the race. Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 69c5b15..5bfdd0c 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -691,7 +691,7 @@ static int bond_check_dev_link(struct bonding *bond, > struct net_device *slave_dev, int reporting) > { > const struct net_device_ops *slave_ops = slave_dev->netdev_ops; >- static int (*ioctl)(struct net_device *, struct ifreq *, int); >+ int (*ioctl)(struct net_device *, struct ifreq *, int); > struct ifreq ifr; > struct mii_ioctl_data *mii; > > > > >-- >Jiri Bohac <jbohac@suse.cz> >SUSE Labs, SUSE CZ > -- 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
From: Jiri Bohac <jbohac@suse.cz> Date: Wed, 21 Oct 2009 15:03:01 +0200 > Hi, > > In mii monitor mode, bond_check_dev_link() calls the the ioctl > handler of slave devices. It stores the ndo_do_ioctl function > pointer to a static (!) ioctl variable and later uses it to call the > handler with the IOCTL macro. > > If another thread executes bond_check_dev_link() at the same time > (even with a different bond, which none of the locks prevent), a > race condition occurs. If the two racing slaves have different > drivers, this may result in one driver's ioctl handler being > called with a pointer to a net_device controlled with a different > driver, resulting in unpredictable breakage. > > Unless I am overlooking something, the "static" must be a > copy'n'paste error (?). > > > Signed-off-by: Jiri Bohac <jbohac@suse.cz> Cur and paste... from where? If you look at the 2.6.14-->2.6.14.1 commit in the history-2.6 tree (5db5272c) this static was there from the moment the link status checking got added to the bonding driver in Linus's tree. Nevertheless indeed it is an awful bug, patch applied, thanks! -- 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 69c5b15..5bfdd0c 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -691,7 +691,7 @@ static int bond_check_dev_link(struct bonding *bond, struct net_device *slave_dev, int reporting) { const struct net_device_ops *slave_ops = slave_dev->netdev_ops; - static int (*ioctl)(struct net_device *, struct ifreq *, int); + int (*ioctl)(struct net_device *, struct ifreq *, int); struct ifreq ifr; struct mii_ioctl_data *mii;
Hi, In mii monitor mode, bond_check_dev_link() calls the the ioctl handler of slave devices. It stores the ndo_do_ioctl function pointer to a static (!) ioctl variable and later uses it to call the handler with the IOCTL macro. If another thread executes bond_check_dev_link() at the same time (even with a different bond, which none of the locks prevent), a race condition occurs. If the two racing slaves have different drivers, this may result in one driver's ioctl handler being called with a pointer to a net_device controlled with a different driver, resulting in unpredictable breakage. Unless I am overlooking something, the "static" must be a copy'n'paste error (?). Signed-off-by: Jiri Bohac <jbohac@suse.cz>