netdev - Re: Using a waiting MDIO does not go well with a spinlocked bridge

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]

Date:	Mon, 23 Mar 2015 07:45:52 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	Scott Feldman <sfeldma@...il.com>
Cc:	Jonas Johansson <jonasj76@...il.com>,
	Netdev <netdev@...r.kernel.org>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: Using a waiting MDIO does not go well with a spinlocked bridge

Fri, Mar 20, 2015 at 07:46:47PM CET, sfeldma@...il.com wrote:
>On Fri, Mar 20, 2015 at 5:22 AM, Jonas Johansson <jonasj76@...il.com> wrote:
>> The bridge code will sometimes hold a spinlock and the code following must
>> therefore be atomic. If using a MDIO call which uses a wait/sleep in this
>> contex, the kernel will not be very happy.
>>
>> I'm using a switch device and wants to flush its FDB when the linux bridge
>> FDB is flushed. I've implemented some hooks for this task.
>> In short:
>>      bridge    - br_fdb_flush() & br_fdb_delete_by_port
>>   -> switchdev - switch_flush()
>>   -> dsa       - slave_flush()
>>   -> mv88e6xxx - mv88_flush()
>
>I think we need to hook switchdev in fdb_delete(), then it'll get
>called from flush and ageing out operations, rather than adding a new
>switch_flush().  But, that's an aside for your main issue that the
>bridge will hold a spinlock for most (all?) FDB delete operations.  I
>don't see a way around relaxing that, on the bridge side, since it's
>doing things like walking lists while deleting list elements.  So that
>means the call into switchdev will be spinlocked, so switchdev driver
>needs to deal with that.  Scheduling to work queue is one option, as
>you mention, if FDB delete can't be done under the spinlock.


I agree that removing/changing spinlock in bridge code is no-go. Driver
should deal with running callback in atomic context itself.

>
>
>> So, when a bridge port is flushed via e.g. sysfs, the mv88_flush() function
>> will at the end be called. The mv88_flush() will use MDIO calls to set the
>> proper registers and flush the device. But, due to that the MDIO on my
>> platform uses wait_for_completion() and a spinlock is held (in this case in
>> brport_store()) the process will not go very well.
>>
>> The only possible solutions that came into my mind is:
>>  1) Let mv88_flush() schedule a work queue to take care of the flush
>>     later on.
>>  2) Change the MDIO implementation to use polling.
>>  3) Dont use spinlock in bridge code.
>>
>> 1) Using this approach the the atomic part is missed, i.e. the switch device
>> isn't guaranteed to be flushed after the command has been issued. And, if a
>> FDB entry is added (atomic) to the switch device immediately after the flush
>> command, there will not be defined if the entry will be added before or
>> after the flush occurs. To solve this, all (FDB) operations must be added to
>> a work queue to assure that they are executed in the right order.
>
>We would loose the FDB add results if added to work queue.  On add,
>you could check work queue delete list for entry, and if there, remove
>from work queue list.
>
>>
>> 2) This will result in unsued CPU cycles.
>>
>> 3) Havent looked into this, but probably a lot of work.
>
>Can of worms...wouldn't recommend that option.
>
>> Any ideas?
--
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