2008-03-21 13:53:14

by Libor Pechacek

Subject: [PATCH] bonding: Fix sysfs attribute handling

bonding: Fix sysfs attribute handling

For bonding interfaces any attempt to read the sysfs directory contents after
module removal results in an oops. The fix is to release sysfs attributes
for the interfaces upon module unload.

Signed-off-by: Libor Pechacek <[email protected]>
--
drivers/net/bonding/bond_main.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 0942d82..33767d4 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4528,8 +4528,7 @@ static void bond_free_all(void)
netif_tx_unlock_bh(bond_dev);
/* Release the bonded slaves */
bond_release_all(bond_dev);
- bond_deinit(bond_dev);
- unregister_netdevice(bond_dev);
+ bond_destroy(bond);
}

#ifdef CONFIG_PROC_FS

2008-03-23 00:27:55

by Andrew Morton

Subject: Re: [PATCH] bonding: Fix sysfs attribute handling

On Fri, 21 Mar 2008 14:52:55 +0100 Libor Pechacek <[email protected]> wrote:

> bonding: Fix sysfs attribute handling
>
> For bonding interfaces any attempt to read the sysfs directory contents after
> module removal results in an oops. The fix is to release sysfs attributes
> for the interfaces upon module unload.
>
> Signed-off-by: Libor Pechacek <[email protected]>
> --
> drivers/net/bonding/bond_main.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 0942d82..33767d4 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4528,8 +4528,7 @@ static void bond_free_all(void)
> netif_tx_unlock_bh(bond_dev);
> /* Release the bonded slaves */
> bond_release_all(bond_dev);
> - bond_deinit(bond_dev);
> - unregister_netdevice(bond_dev);
> + bond_destroy(bond);
> }

Is this also needed in 2.6.24.x?

2008-03-23 01:12:50

by Jay Vosburgh

Subject: Re: [PATCH] bonding: Fix sysfs attribute handling

Andrew Morton <[email protected]> wrote:

>On Fri, 21 Mar 2008 14:52:55 +0100 Libor Pechacek <[email protected]> wrote:
>
>> bonding: Fix sysfs attribute handling
>>
>> For bonding interfaces any attempt to read the sysfs directory contents after
>> module removal results in an oops. The fix is to release sysfs attributes
>> for the interfaces upon module unload.
>>
>> Signed-off-by: Libor Pechacek <[email protected]>
>> --
>> drivers/net/bonding/bond_main.c | 3 +--
>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 0942d82..33767d4 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4528,8 +4528,7 @@ static void bond_free_all(void)
>> netif_tx_unlock_bh(bond_dev);
>> /* Release the bonded slaves */
>> bond_release_all(bond_dev);
>> - bond_deinit(bond_dev);
>> - unregister_netdevice(bond_dev);
>> + bond_destroy(bond);
>> }
>
>Is this also needed in 2.6.24.x?

Yes (I haven't tested it personally, but the affected code is
the same). I was going to forward this to stable when it was accepted
upstream. The oops is fairly straightforward to create:

# modprobe bonding
# cd /sys/class/net/bond0/bonding
# rmmod bonding
# ls

-J

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

2008-03-23 20:05:28

by Libor Pechacek

Subject: Re: [PATCH] bonding: Fix sysfs attribute handling

> Andrew Morton <[email protected]> wrote:
>
>>On Fri, 21 Mar 2008 14:52:55 +0100 Libor Pechacek <[email protected]>
>> wrote:
>>
>>> bonding: Fix sysfs attribute handling
>>>
>>> For bonding interfaces any attempt to read the sysfs directory contents
>>> after
>>> module removal results in an oops. The fix is to release sysfs
>>> attributes
>>> for the interfaces upon module unload.
>>>
>>> Signed-off-by: Libor Pechacek <[email protected]>
>>> --
>>> drivers/net/bonding/bond_main.c | 3 +--
>>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c
>>> b/drivers/net/bonding/bond_main.c
>>> index 0942d82..33767d4 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -4528,8 +4528,7 @@ static void bond_free_all(void)
>>> netif_tx_unlock_bh(bond_dev);
>>> /* Release the bonded slaves */
>>> bond_release_all(bond_dev);
>>> - bond_deinit(bond_dev);
>>> - unregister_netdevice(bond_dev);
>>> + bond_destroy(bond);
>>> }
>>
>>Is this also needed in 2.6.24.x?
>
> Yes (I haven't tested it personally, but the affected code is
> the same). I was going to forward this to stable when it was accepted
> upstream. The oops is fairly straightforward to create:
>
> # modprobe bonding
> # cd /sys/class/net/bond0/bonding
> # rmmod bonding
> # ls

Yes, exactly.

I tested the patch with the latest Linus' tree (GIT commit
49a5ba46c5d1e34bcb07634157b29d7414ce13bd), but I first found the bug in
2.6.16.

Libor