netdev - Re: [PATCH v2] net: fec: Use a spinlock to guard `fep->ptp_clk

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 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, 9 Sep 2022 15:56:33 +0200
From:   Francesco Dolcini <francesco.dolcini@...adex.com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>,
        Csókás Bence <csokas.bence@...lan.hu>
CC:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Richard Cochran <richardcochran@...il.com>,
        kernel@...gutronix.de,
        Francesco Dolcini <francesco.dolcini@...adex.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2] net: fec: Use a spinlock to guard `fep->ptp_clk_on`

On Wed, Sep 07, 2022 at 04:39:15PM +0200, Marc Kleine-Budde wrote:
> On 05.09.2022 09:38:04, Marc Kleine-Budde wrote:
> > On 9/1/22 4:04 PM, Csókás Bence wrote:
> > > Mutexes cannot be taken in a non-preemptible context,
> > > causing a panic in `fec_ptp_save_state()`. Replacing
> > > `ptp_clk_mutex` by `tmreg_lock` fixes this.
> > 
> > I was on holidays, but this doesn't look good.
> 
> Does anyone care to fix this? Csókás?
I do care, however I do not really have resources for anything better than
reverting both of the problematic commits.

If we do not have a fix, I guess this is the only way. Csókás?

For some reason when I tested on imx6 :-( I had no warning, now the
change was merged and while doing a quick test on a imx7
I got new issues.

[   37.061582] ========================================================
[   37.070203] WARNING: possible irq lock inversion dependency detected
[   37.078847] 6.0.0-rc4-00137-g506357871c18 #3 Tainted: G        W
[   37.087944] --------------------------------------------------------
[   37.096615] systemd/580 just changed the state of lock:
[   37.104160] c218f2a4 (&dev->tx_global_lock){+.-.}-{2:2}, at: dev_watchdog+0x18/0x2c4
[   37.116691] but this lock took another, SOFTIRQ-unsafe lock in the past:
[   37.125896]  (&fep->tmreg_lock){+.+.}-{2:2}
[   37.125921]
[   37.125921]
[   37.125921] and interrupts could create inverse lock ordering between them.
[   37.125921]
[   37.153018]
[   37.153018] other info that might help us debug this:
[   37.163824]  Possible interrupt unsafe locking scenario:
[   37.163824]
[   37.174714]        CPU0                    CPU1
[   37.181215]        ----                    ----
[   37.187631]   lock(&fep->tmreg_lock);
[   37.193110]                                local_irq_disable();
[   37.200841]                                lock(&dev->tx_global_lock);
[   37.209153]                                lock(&fep->tmreg_lock);
[   37.217061]   <Interrupt>
[   37.221328]     lock(&dev->tx_global_lock);
[   37.227167]
[   37.227167]  *** DEADLOCK ***

that just goes away reverting this patch.

Francesco