netdev - Re: [PATCH net] bridge: fix hello and hold timers starting/stopping

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+Nub0bpM1ez-SBdqz3zzFDZYVRVNprEop=pLwhveZ3_wxU=Qg@mail.gmail.com>
Date:   Fri, 19 May 2017 19:06:21 +0200
From:   Ivan Vecera <cera@...a.cz>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Cc:     network dev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, sashok@...ulusnetworks.com,
        Stephen Hemminger <stephen@...workplumber.org>,
        bridge@...ts.linux-foundation.org, Xin Long <lucien.xin@...il.com>
Subject: Re: [PATCH net] bridge: fix hello and hold timers starting/stopping

2017-05-19 18:55 GMT+02:00 Nikolay Aleksandrov <nikolay@...ulusnetworks.com>:
> On 5/19/17 7:25 PM, Ivan Vecera wrote:
>>
>> Current bridge code incorrectly handles starting/stopping of hello and
>> hold timers during STP enable/disable.
>>
>> 1. Timers are stopped in br_stp_start() during NO_STP->USER_STP
>>     transition. This is not correct as the timers are stopped in NO_STP
>>     case.
>
>
> This really is a noop, but ok.

Yes, stopping of stopped timers are safe but confusing.

>> 2. Timers are started in br_stp_stop() during USER_STP->NO_STP transition.
>>     This is not also correct as the timers should be stopped in NO_STP
>>     state.
>
>
> Indeed, but the actual end result is almost as them being stopped because
> in the timers there are specific checks if the STP == KERNEL_STP (see
> br_transmit_config()) and the hold_timers will simply expire and not rearm
> in any other mode. The only real problem is the hello_timer which continues
> to rearm itself, but with Xin's earlier patch that is taken care of too.

Yes, this is clean-up as well. The starting of timers are more
confusing than dangerous
but from a reader's point of view the starting of timers is non-sense
when STP is
going to be disabled.

>>
>> 3. Timers are NOT stopped in br_stp_stop() during KERNEL_STP->NO_STP
>>     transition. They should be stopped as they are running in KERNEL_STP
>>     state and should not run in NO_STP case.
>
>
> Same comment as for point 2.
This can be removed... and leave hello_timer handler to stop itself.

>> The patch is a follow-up for "bridge: start hello_timer when enabling
>> KERNEL_STP in br_stp_start" patch from Xin Long.
>>
>
> I'd say this is more of a cleanup/improvement after Xin's patch and thus
> would
> suggest targeting net-next. The only real issue is fixed by his patch.
Agree... will send resend against net-next.

Thanks for comments,
Ivan