netdev - Re: [PATCH v3 03/22] netconsole: Introduce 'enabled' state-machine

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <AANLkTikG-XepYDb3R9=T8NP5TWVgsKNK-QmXrznGDHUO@mail.gmail.com>
Date:	Fri, 17 Dec 2010 13:20:30 -0800
From:	Mike Waychison <mikew@...gle.com>
To:	Neil Horman <nhorman@...driver.com>
Cc:	simon.kagstrom@...insight.net, davem@...emloft.net,
	Matt Mackall <mpm@...enic.com>, adurbin@...gle.com,
	linux-kernel@...r.kernel.org, chavey@...gle.com,
	Greg KH <greg@...ah.com>, netdev@...r.kernel.org,
	Américo Wang <xiyou.wangcong@...il.com>,
	akpm@...ux-foundation.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v3 03/22] netconsole: Introduce 'enabled' state-machine

On Fri, Dec 17, 2010 at 12:52 PM, Neil Horman <nhorman@...driver.com> wrote:
> On Tue, Dec 14, 2010 at 01:29:10PM -0800, Mike Waychison wrote:

>> @@ -680,16 +699,17 @@ static int netconsole_netdev_event(struct notifier_block *this,
>>       struct net_device *dev = ptr;
>>
>>       if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
>> -           event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
>> +           event == NETDEV_BONDING_DESLAVE))
>>               goto done;
>>
>>       spin_lock_irqsave(&target_list_lock, flags);
>>       list_for_each_entry(nt, &target_list, list) {
>> -             if (nt->np.dev == dev) {
>> +             if (nt->np_state == NETPOLL_ENABLED && nt->np.dev == dev) {
>>                       switch (event) {
>>                       case NETDEV_CHANGENAME:
>>                               strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
>>                               break;
>> +                     case NETDEV_BONDING_DESLAVE:
>>                       case NETDEV_UNREGISTER:
> I don't follow what you're doing here.  Previously NETDEV_BONDING_DESLAVE events
> simply caused the netconsole interface to get deactivated, and now we're doing a
> dev_put on the bonded interface bacause of the above move.  Given that
> NETDEV_BONDING_DESLAVE events are generated when a slave leaves the bond, this
> doesn't seem right, or am I missing something

Ya, I did a poor job of explaining this bit.  Let me try now:

- NETDEV_GOING_DOWN is unneccesary because we will always see a
NETDEV_UNREGISTER event, which is why it is removed.
- I wasn't sure how to handle the NETDEV_BONDING_DESLAVE event in a
consistent way.  Originally we would disable the target, but this
actually leaks nt->np as it will never get cleaned up properly.  To
the user, it looks as if the target gets disabled.

I don't think you have any contention with the first comment above
(though I need to document it).

You are right in that with this patch, the target is left enabled,
when we should probably mark it as disabled.  I'd be happy to add the
transition from NETPOLL_ENABLED -> NETPOLL_DISABLED here if you'd
like, but it'd move again in the next patch anyway (which already
re-introduces the state transition in defer_netpoll_cleanup()).  Sorry
this is confusing :(   I'll make it clearer in the next series
version.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html