netdev - Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races

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: <20171208161714.GC30846@lunn.ch>
Date:   Fri, 8 Dec 2017 17:17:14 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC 0/4] Fixes for Marvell MII paged register access races

Hi Russell

> There is an open question whether there should be generic helpers for
> this.  Generic helpers would mean:
> 
> - Additional couple of function pointers in phy_driver to read/write the
>   paging register.  This has the restriction that there must only be one
>   paging register.

I must be missing something. I don't see why there is this
restriction. Don't we just need

int phy_get_page(phydev);
int phy_set_page(phydev, page);

We might even be able to do without phy_get_page(), if we assume page
0 should be selected by default, and all paged reads/write return to
page 0 afterwards.

> - The helpers become more expensive, and because they're in a separate
>   compilation unit, the compiler will be unable to optimise them by
>   inlining the static functions.

The mdio bus is slow. Often there is a completion function triggered
by an interrupt etc. Worst case it is bit-banging. I suspect the gains
from inlining are just noise in the bigger picture.

> - The helpers would be re-usable, saving replications of that code, and
>   making it more likely for phy authors to safely access the PHY.

This is the key point for me. This has likely to of been broken for
years. If it is obviously broken, driver writers are more likely to
get it right. Here it is not obvious, so we should take it out of the
driver writers hands and do it in the core.

> Another potential question is whether using the mdiobus lock (which
> excludes all other MII bus access) is best - while it has the advantage
> of also ensuring atomicity with userspace accesses, it means that no one
> else can access an independent PHY on the same bus while a paged access
> is on-going.  It feels like a big hammer, but I'm not convinced that we
> will see a lot of contention on it.

I think you are right about there being little contention on the lock.
I suspect most paged accesses are performed during initial setup and
configuration. I guess the once per second poll does not need to use
paged registers. And the weight of that hammer can be reduced a lot by
using interrupts instead of polling.

      Andrew