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 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: <20220907143915.5w65kainpykfobte@pengutronix.de>
Date:   Wed, 7 Sep 2022 16:39:15 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Csókás Bence <csokas.bence@...lan.hu>,
        netdev@...r.kernel.org
Cc:     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 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?

> > @@ -2036,15 +2037,15 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
> >  			return ret;
> >  
> >  		if (fep->clk_ptp) {
> > -			mutex_lock(&fep->ptp_clk_mutex);
> > +			spin_lock_irqsave(&fep->tmreg_lock, flags);
> >  			ret = clk_prepare_enable(fep->clk_ptp);
> 
> clock_prepare() (and thus clk_prepare_enable()) must not be called from atomic
> context.

You cannot call clk_prepare_enable() from atomic context. If you compile
your kernel with lockdep, you'll get this splat:

| [    5.907789] BUG: sleeping function called from invalid context at drivers/net/ethernet/freescale/fec_main.c:2071
| [    5.918140] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 1, name: swapper/0
| [    5.926181] preempt_count: 1, expected: 0                                                    
| [    5.930223] 2 locks held by swapper/0/1:                                                     
| [    5.934180]  #0: c41a4094 (&dev->mutex){....}-{4:4}, at: __driver_attach+0x8c/0x150
| [    5.941968]  #1: c42439b0 (&fep->tmreg_lock){....}-{3:3}, at: fec_enet_clk_enable+0x5c/0x25c
| [    5.950533] irq event stamp: 124698                                                          
| [    5.954052] hardirqs last  enabled at (124697): [<c0f6c6dc>] _raw_spin_unlock_irqrestore+0x58/0x6c
| [    5.963058] hardirqs last disabled at (124698): [<c0f6c490>] _raw_spin_lock_irqsave+0x88/0xa4
| [    5.971622] softirqs last  enabled at (122454): [<c02b47bc>] bdi_register_va+0x1ac/0x1d8                                                                                                     
| [    5.979753] softirqs last disabled at (122452): [<c02b4738>] bdi_register_va+0x128/0x1d8

| $ head -2071 drivers/net/ethernet/freescale/fec_main.c | tail -15
| 
| static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
| {
|         struct fec_enet_private *fep = netdev_priv(ndev);
|         unsigned long flags;
|         int ret;
| 
|         if (enable) {
|                 ret = clk_prepare_enable(fep->clk_enet_out);
|                 if (ret)
|                         return ret;
| 
|                 if (fep->clk_ptp) {
|                         spin_lock_irqsave(&fep->tmreg_lock, flags);
|                         ret = clk_prepare_enable(fep->clk_ptp);

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)