linux-kernel - Re: [PATCH net] net: dsa: mv88e6xxx: lock mutex when freeing IRQs

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: <20170926180136.GD3325@lunn.ch>
Date:   Tue, 26 Sep 2017 20:01:36 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...oirfairelinux.com,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net] net: dsa: mv88e6xxx: lock mutex when freeing IRQs

On Tue, Sep 26, 2017 at 01:48:37PM -0400, Vivien Didelot wrote:
> mv88e6xxx_g2_irq_free locks the registers mutex, but not
> mv88e6xxx_g1_irq_free, which results in a stack trace from
> assert_reg_lock when unloading the mv88e6xxx module. Fix this.
> 
> Fixes: 3460a5770ce9 ("net: dsa: mv88e6xxx: Mask g1 interrupts and free interrupt")
> Signed-off-by: Vivien Didelot <vivien.didelot@...oirfairelinux.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index c6678aa9b4ef..b4359e4e5165 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -338,9 +338,11 @@ static void mv88e6xxx_g1_irq_free(struct mv88e6xxx_chip *chip)
>  	int irq, virq;
>  	u16 mask;
>  
> +	mutex_lock(&chip->reg_lock);
>  	mv88e6xxx_g1_read(chip, MV88E6XXX_G1_CTL1, &mask);
>  	mask |= GENMASK(chip->g1_irq.nirqs, 0);
>  	mv88e6xxx_g1_write(chip, MV88E6XXX_G1_CTL1, mask);
> +	mutex_unlock(&chip->reg_lock);
>  
>  	free_irq(chip->irq, chip);
>  

Hi Vivien

static int mv88e6xxx_probe(struct mdio_device *mdiodev)
{

...

out_g1_irq:
	if (chip->irq > 0) {
		mutex_lock(&chip->reg_lock);
		mv88e6xxx_g1_irq_free(chip);
		mutex_unlock(&chip->reg_lock);
	}

It looks like this will deadlock?

In general, i tried to keep the mutex out of the interrupt code.  That
is not totally possible, the IRQ thread handler needs it. But
otherwise, the IRQ code assumes it is called with the mutex taken.

So i think it is better to hold the mutex in mv88e6xxx_remove() when
calling mv88e6xxx_g1_irq_free().

   Andrew