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]

Date:   Fri, 27 Aug 2021 20:34:33 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...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 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.

> 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.

> 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.

	Andrew