netdev - Re: [PATCH net] net: dsa: mv88e6xxx: stop calling irq_domain_add_simple with the reg

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: <20210827184525.p44pir5or4h5nwgk@skbuf>
Date:   Fri, 27 Aug 2021 21:45:25 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net] net: dsa: mv88e6xxx: stop calling
 irq_domain_add_simple with the reg_lock held

On Fri, Aug 27, 2021 at 08:34:33PM +0200, Andrew Lunn wrote:
> On Fri, Aug 27, 2021 at 09:01:01PM +0300, Vladimir Oltean wrote:
> > The mv88e6xxx IRQ setup code has some pretty horrible locking patterns,
> > and wrong.
>
> I agree about the patterns. But it has been lockdep clean, i spent a
> while testing it, failed probes, unloads etc, and adding comments.
>
> I suspect it is now wrong because of core changes.

It's true, it is lockdep-clean the way it is structured now, but I
suspect that is purely by chance. I had to shift code around a bit to
get lockdep to shout, my bad for not really mentioning it: I moved
mv88e6xxx_mdios_register from mv88e6xxx_probe to mv88e6xxx_setup, all in
all a relatively superficial change (I am trying to test something out),
hence the reason why I did not believe it would make such a huge
difference.

I realize now it puts you in a bad light since it suggests you didn't
test it with lockdep, and I apologize.

> > Only hardware access should need the register lock, and this in itself
> > is for the mv88e6xxx_smi_indirect_ops to work properly and nothing more,
> > unless I'm misunderstanding something
>
> Historically, reg_lock has been used to serialize all access to the
> hardware across entries points into the driver. Not everything takes
> rtnl lock. Clearly, interrupts don't. I don't know if PTP takes it. In
> the past there was been hwmon code, etc. The reg_lock is used to
> serialize all this. The patterns of all entry points into the driver
> taking the lock has in general worked well. Just interrupt code is a
> pain.

I empathize with working in the blind w.r.t. locking, when rtnl_mutex
covers everything. As you point out, threaded interrupts do not the
rtnl_lock, so that is a good opportunity to analyze what needs serialization,
which I do not have on sja1105. Nonetheless, my experience is that
hardware is a pretty parallel/reentrant beast, a "register lock" is
almost always the wrong answer. IMHO, proper locking would need to find
what are the sequences of SMI reads/writes that need to be indivisible,
and only lock around those, and lock per topic if possible.

> > Fixes: dc30c35be720 ("net: dsa: mv88e6xxx: Implement interrupt support.")
>
> As i said, i suspect this is the wrong commit. You need to look at
> changes to the interrupt core. There is even a danger that if this
> gets backported too far, it could add deadlocks.

Ok, retarget to "net-next"?