Re: [PATCH] bonding: bond_slave_info_query() fix

[Jay Vosburgh <fubar@us.ibm.com>]

Eric Dumazet <dada1@cosmosbay.com> wrote:

>Jiri Pirko a écrit :
>> Thu, Apr 23, 2009 at 01:24:07PM CEST, dada1@cosmosbay.com wrote:
>>> bond_slave_info_query() should keep a read lock while accessing slave info,
>>> or risk accessing stale data and corruption.
>>>
>>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 63369b6..66697db 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -2213,33 +2213,27 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in
>>> {
>>> 	struct bonding *bond = netdev_priv(bond_dev);
>>> 	struct slave *slave;
>>> -	int i, found = 0;
>>> +	int i, ret = -ENODEV;
>>>
>>> -	if (info->slave_id < 0) {
>>> +	if (info->slave_id < 0)
>>> 		return -ENODEV;
>> 
>> You can return ret; here since -ENODEV is there, but I don't know if it isn't
>> against policy. Patch looks good anyway.
>>
>
>Yes, we can just delete this extra test, since requested slave_id wont
>be found anyway if negative.
>
>Thanks
>
>[PATCH] bonding: bond_slave_info_query() fix
>
>bond_slave_info_query() should keep a read lock while accessing slave info,
>or risk accessing stale data and corruption.

	I don't think that's actually true, since changes to the data
being referenced are (should be) protected by RTNL, and
bond_slave_info_query runs via ioctl, and is holding RTNL.

	Nevertheless, the patch does make the function cleaner, so I
have no objections.

	-J

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

>Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 63369b6..67515b7 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2213,33 +2213,24 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in
> {
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct slave *slave;
>-	int i, found = 0;
>-
>-	if (info->slave_id < 0) {
>-		return -ENODEV;
>-	}
>+	int i, res = -ENODEV;
>
> 	read_lock(&bond->lock);
>
> 	bond_for_each_slave(bond, slave, i) {
> 		if (i == (int)info->slave_id) {
>-			found = 1;
>+			res = 0;
>+			strcpy(info->slave_name, slave->dev->name);
>+			info->link = slave->link;
>+			info->state = slave->state;
>+			info->link_failure_count = slave->link_failure_count;
> 			break;
> 		}
> 	}
>
> 	read_unlock(&bond->lock);
>
>-	if (found) {
>-		strcpy(info->slave_name, slave->dev->name);
>-		info->link = slave->link;
>-		info->state = slave->state;
>-		info->link_failure_count = slave->link_failure_count;
>-	} else {
>-		return -ENODEV;
>-	}
>-
>-	return 0;
>+	return res;
> }
>
> /*-------------------------------- Monitoring -------------------------------*/
>
>--
>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

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com