Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Sat, 30 Sep 2023 18:04:28 +0200
From:   Simon Horman <horms@kernel.org>
To:     Chengfeng Ye <dg573847474@gmail.com>
Cc:     3chas3@gmail.com, davem@davemloft.net,
        linux-atm-general@lists.sourceforge.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH] atm: solos-pci: Fix potential deadlock on
 &cli_queue_lock and &tx_queue_lock
Message-ID: <20230930160428.GB92317@kernel.org>
References: <20230926104442.8684-1-dg573847474@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230926104442.8684-1-dg573847474@gmail.com>
Precedence: bulk

+ David Woodhouse <dwmw2@infradead.org>

On Tue, Sep 26, 2023 at 10:44:42AM +0000, Chengfeng Ye wrote:
> As &card->cli_queue_lock and &card->tx_queue_lock are acquired under
> softirq context along the following call chain from solos_bh(), other
> acquisition of the same lock inside process context should disable
> at least bh to avoid double lock.
> 
> <deadlock #1>
> console_show()
> --> spin_lock(&card->cli_queue_lock)
> <interrupt>
>    --> solos_bh()
>    --> spin_lock(&card->cli_queue_lock)
> 
> <deadlock #2>
> pclose()
> --> spin_lock(&card->tx_queue_lock)
> <interrupt>
>    --> solos_bh()
>    --> fpga_tx()
>    --> spin_lock(&card->tx_queue_lock)
> 
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
> 
> To prevent the potential deadlock, the patch uses spin_lock_irqsave()
> on the two locks under process context code consistently to prevent
> the possible deadlock scenario.

Hi Chengfeng Ye,

thanks for your patch.

As this patch seems to fix two, albeit, similar problems,
it should probably be split into two patches.

As fixes for Networking code they should probably be targeted at the
'net' tree. Which should be denoted in the subject.

	Subject: [PATCH net] ...

And as fixes the patch(es) should probably have Fixes tags.
These ones seem appropriate to me, but I could be wrong.

Fixes: 9c54004ea717 ("atm: Driver for Solos PCI ADSL2+ card.")
Fixes: 213e85d38912 ("solos-pci: clean up pclose() function")

> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
> ---
>  drivers/atm/solos-pci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
> index 94fbc3abe60e..247e9200e312 100644
> --- a/drivers/atm/solos-pci.c
> +++ b/drivers/atm/solos-pci.c
> @@ -447,11 +447,12 @@ static ssize_t console_show(struct device *dev, struct device_attribute *attr,
>  	struct atm_dev *atmdev = container_of(dev, struct atm_dev, class_dev);
>  	struct solos_card *card = atmdev->dev_data;
>  	struct sk_buff *skb;
> +	unsigned long flags;
>  	unsigned int len;
>  
> -	spin_lock(&card->cli_queue_lock);
> +	spin_lock_irqsave(&card->cli_queue_lock, flags);
>  	skb = skb_dequeue(&card->cli_queue[SOLOS_CHAN(atmdev)]);
> -	spin_unlock(&card->cli_queue_lock);
> +	spin_unlock_irqrestore(&card->cli_queue_lock, flags);
>  	if(skb == NULL)
>  		return sprintf(buf, "No data.\n");
>  
> @@ -954,16 +955,17 @@ static void pclose(struct atm_vcc *vcc)
>  	unsigned char port = SOLOS_CHAN(vcc->dev);
>  	struct sk_buff *skb, *tmpskb;
>  	struct pkt_hdr *header;
> +	unsigned long flags;
>  
>  	/* Remove any yet-to-be-transmitted packets from the pending queue */
> -	spin_lock(&card->tx_queue_lock);
> +	spin_lock_irqsave(&card->tx_queue_lock, flags);
>  	skb_queue_walk_safe(&card->tx_queue[port], skb, tmpskb) {
>  		if (SKB_CB(skb)->vcc == vcc) {
>  			skb_unlink(skb, &card->tx_queue[port]);
>  			solos_pop(vcc, skb);
>  		}
>  	}
> -	spin_unlock(&card->tx_queue_lock);
> +	spin_unlock_irqrestore(&card->tx_queue_lock, flags);
>  
>  	skb = alloc_skb(sizeof(*header), GFP_KERNEL);
>  	if (!skb) {
> -- 
> 2.17.1
> 
> 

-- 
pw-bot: changes-requested