[PATCH] bonding: fix a race condition in calls to slave MII ioctls

[PATCH] bonding: fix a race condition in calls to slave MII ioctls

[Jiri Bohac]

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>

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

Re: [PATCH] bonding: fix a race condition in calls to slave MII ioctls

[Jay Vosburgh]

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
>

Re: [PATCH] bonding: fix a race condition in calls to slave MII ioctls

[David Miller]

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!