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]

Message-ID: <20150321063735.GB29867@roeck-us.net>
Date:	Fri, 20 Mar 2015 23:37:35 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Andrew Lunn <andrew@...n.ch>
Cc:	Jonas Johansson <jonasj76@...il.com>, netdev@...r.kernel.org,
	stephen@...workplumber.org, f.fainelli@...il.com, jiri@...nulli.us,
	sfeldma@...il.com
Subject: Re: Using a waiting MDIO does not go well with a spinlocked bridge

On Fri, Mar 20, 2015 at 02:16:22PM +0100, Andrew Lunn wrote:
> On Fri, Mar 20, 2015 at 01:22:46PM +0100, Jonas Johansson 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()
> 
> Hi Jonas
> 
> Have you seen the patches from Guenter Roeck implementing hardware
> bridging? There should be a new version coming out soon.
> 
> > 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.
> 
> Ah, not good. We have a number of mutex in the mv88x6xxx code, one of
> which is used with fdb operations..
>  
The mutexes in the mv88x6xxx don't matter, really. There is also the
mdio bus mutex which would kill us anyway.

Handling port state changes is already implemented with a workqueue
in my code because of the spinlock problem. I use that same workqueue
in my (not submitted) patch to flush the fdb.

Guenter

> > 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.
> 
> I don't think these is feasible. The MDIO bus could be a gpio
> bit-banging interface. It is hard to guarantee that the GPIO code will
> not sleep.
> 
> >  3) Dont use spinlock in bridge code.
> 
> This would be my preference, but i've no idea how much work it is.  We
> should audit the bridge code and document in what context operations
> on the switch are called.
>
--
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