netdev - Re: [PATCH 0/3] bonding: 3 fixes for 2.6.24

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 14 Jan 2008 23:15:47 +0100 (CET)
From:	Krzysztof Oledzki <olel@....pl>
To:	Jay Vosburgh <fubar@...ibm.com>
cc:	Andy Gospodarek <andy@...yhouse.net>, netdev@...r.kernel.org,
	Jeff Garzik <jgarzik@...ox.com>,
	David Miller <davem@...emloft.net>,
	Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH 0/3] bonding: 3 fixes for 2.6.24 



On Sat, 12 Jan 2008, Jay Vosburgh wrote:

> Krzysztof Oledzki <olel@....pl> wrote:
> [...]
>> Exactly. All I need to do is to reboot my server, I have 100% probability
>> to get the warning.
>
> 	I wish it were that easy for me; I'm not sure what magic thing
> you've got on your server or network that I don't, but I haven't been
> able to make this lockdep warning happen at all.
>
>> Right. So, what is the final patch? I would like to test it if that's
>> possible. ;)
>
> 	Can you test the following and let me know if it triggers the
> warning?  I believe this is the minimum locking needed, and based on
> input from Herbert, we shouldn't need to hold the lock at _bh.  If this
> one works, and nobody sees any other issues with it, then it's the final
> patch for this lockdep problem.  I'll add some deep, meaningful comments
> to explain the locking a bit (i.e., we're called with rtnl for the
> allmulti and promisc cases, so we're ok there without additional locks,
> but the later code could be called from anywhere, so it needs locks to
> prevent the slave list from changing, but the mc_lists themselves are
> covered by the netif_tx_lock that all callers will hold), but this would
> be the actual code change.
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 77d004d..6906dbc 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3937,8 +3937,6 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
> 	struct bonding *bond = bond_dev->priv;
> 	struct dev_mc_list *dmi;
>
> -	write_lock_bh(&bond->lock);
> -
> 	/*
> 	 * Do promisc before checking multicast_mode
> 	 */
> @@ -3959,6 +3957,8 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
> 		bond_set_allmulti(bond, -1);
> 	}
>
> +	read_lock(&bond->lock);
> +
> 	bond->flags = bond_dev->flags;
>
> 	/* looking for addresses to add to slaves' mc list */
> @@ -3979,7 +3979,7 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
> 	bond_mc_list_destroy(bond);
> 	bond_mc_list_copy(bond_dev->mc_list, bond, GFP_ATOMIC);
>
> -	write_unlock_bh(&bond->lock);
> +	read_unlock(&bond->lock);
> }
>
> /*
>
>

I can confirm that the warning went away.

Tested-by: Krzysztof Piotr Oledzki <ole@....pl>

Best regards,

 				Krzysztof Olędzki