* [PATCH] bonding: bond_slave_info_query() fix
@ 2009-04-23 11:24 Eric Dumazet
2009-04-23 12:11 ` Jiri Pirko
2009-05-01 22:16 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2009-04-23 11:24 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List, Jay Vosburgh
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;
- }
read_lock(&bond->lock);
bond_for_each_slave(bond, slave, i) {
if (i == (int)info->slave_id) {
- found = 1;
+ ret = 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 ret;
}
/*-------------------------------- Monitoring -------------------------------*/
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_slave_info_query() fix
2009-04-23 11:24 [PATCH] bonding: bond_slave_info_query() fix Eric Dumazet
@ 2009-04-23 12:11 ` Jiri Pirko
2009-04-23 13:39 ` Eric Dumazet
2009-05-01 22:16 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Jiri Pirko @ 2009-04-23 12:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Linux Netdev List, Jay Vosburgh
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.
>- }
>
> read_lock(&bond->lock);
>
> bond_for_each_slave(bond, slave, i) {
> if (i == (int)info->slave_id) {
>- found = 1;
>+ ret = 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 ret;
> }
>
> /*-------------------------------- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_slave_info_query() fix
2009-04-23 12:11 ` Jiri Pirko
@ 2009-04-23 13:39 ` Eric Dumazet
2009-04-23 14:51 ` Jay Vosburgh
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2009-04-23 13:39 UTC (permalink / raw)
To: Jiri Pirko; +Cc: David S. Miller, Linux Netdev List, Jay Vosburgh
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.
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 -------------------------------*/
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_slave_info_query() fix
2009-04-23 13:39 ` Eric Dumazet
@ 2009-04-23 14:51 ` Jay Vosburgh
2009-05-01 22:17 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Jay Vosburgh @ 2009-04-23 14:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jiri Pirko, David S. Miller, Linux Netdev List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_slave_info_query() fix
2009-04-23 11:24 [PATCH] bonding: bond_slave_info_query() fix Eric Dumazet
2009-04-23 12:11 ` Jiri Pirko
@ 2009-05-01 22:16 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2009-05-01 22:16 UTC (permalink / raw)
To: dada1; +Cc: netdev, fubar
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Thu, 23 Apr 2009 13:24:07 +0200
> 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>
Applied, thanks Eric.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bonding: bond_slave_info_query() fix
2009-04-23 14:51 ` Jay Vosburgh
@ 2009-05-01 22:17 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2009-05-01 22:17 UTC (permalink / raw)
To: fubar; +Cc: dada1, jpirko, netdev
From: Jay Vosburgh <fubar@us.ibm.com>
Date: Thu, 23 Apr 2009 07:51:36 -0700
> Eric Dumazet <dada1@cosmosbay.com> wrote:
>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
>
>>Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
Aha, I'll apply this updated version instead, ignore my
other email.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-05-01 22:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-23 11:24 [PATCH] bonding: bond_slave_info_query() fix Eric Dumazet
2009-04-23 12:11 ` Jiri Pirko
2009-04-23 13:39 ` Eric Dumazet
2009-04-23 14:51 ` Jay Vosburgh
2009-05-01 22:17 ` David Miller
2009-05-01 22:16 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).