linux-kernel - Re: [PATCH] IB/hfi1: Fix potential deadlock on &sde->flushlist

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230704114849.GA6455@unreal>
Date:   Tue, 4 Jul 2023 14:48:49 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Chengfeng Ye <dg573847474@...il.com>
Cc:     dennis.dalessandro@...nelisnetworks.com, jgg@...pe.ca,
        linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] IB/hfi1: Fix potential deadlock on &sde->flushlist_lock

On Wed, Jun 28, 2023 at 04:59:25AM +0000, Chengfeng Ye wrote:
> As &sde->flushlist_lock is acquired by timer sdma_err_progress_check()
> through layer of calls under softirq context, other process
> context code acquiring the lock should disable irq.
> 
> Possible deadlock scenario
> sdma_send_txreq()
>     -> spin_lock(&sde->flushlist_lock)
>         <timer interrupt>
>         -> sdma_err_progress_check()
>         -> __sdma_process_event()
>         -> sdma_set_state()
>         -> sdma_flush()
>         -> spin_lock_irqsave(&sde->flushlist_lock, flags) (deadlock here)
> 
> This flaw was found using an experimental static analysis tool we are
> developing for irq-related deadlock.
> 
> The tentative patch fix the potential deadlock by spin_lock_irqsave().
> 
> Signed-off-by: Chengfeng Ye <dg573847474@...il.com>
> ---
>  drivers/infiniband/hw/hfi1/sdma.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/sdma.c b/drivers/infiniband/hw/hfi1/sdma.c
> index bb2552dd29c1..0431f575c861 100644
> --- a/drivers/infiniband/hw/hfi1/sdma.c
> +++ b/drivers/infiniband/hw/hfi1/sdma.c
> @@ -2371,9 +2371,9 @@ int sdma_send_txreq(struct sdma_engine *sde,
>  	tx->sn = sde->tail_sn++;
>  	trace_hfi1_sdma_in_sn(sde, tx->sn);
>  #endif
> -	spin_lock(&sde->flushlist_lock);
> +	spin_lock_irqsave(&sde->flushlist_lock, flags);
>  	list_add_tail(&tx->list, &sde->flushlist);
> -	spin_unlock(&sde->flushlist_lock);
> +	spin_unlock_irqrestore(&sde->flushlist_lock, flags);
>  	iowait_inc_wait_count(wait, tx->num_desc);
>  	queue_work_on(sde->cpu, system_highpri_wq, &sde->flush_worker);
>  	ret = -ECOMM;

It can't work as exactly after "ret = -ECOMM;" line, there is "goto unlock"
and there hfi1 calls to spin_unlock_irqrestore(..) with same "flags".

Plus, we already in context where interrupts are stopped.

Thanks

> @@ -2459,7 +2459,7 @@ int sdma_send_txlist(struct sdma_engine *sde, struct iowait_work *wait,
>  	*count_out = total_count;
>  	return ret;
>  unlock_noconn:
> -	spin_lock(&sde->flushlist_lock);
> +	spin_lock_irqsave(&sde->flushlist_lock, flags);
>  	list_for_each_entry_safe(tx, tx_next, tx_list, list) {
>  		tx->wait = iowait_ioww_to_iow(wait);
>  		list_del_init(&tx->list);
> @@ -2472,7 +2472,7 @@ int sdma_send_txlist(struct sdma_engine *sde, struct iowait_work *wait,
>  		flush_count++;
>  		iowait_inc_wait_count(wait, tx->num_desc);
>  	}
> -	spin_unlock(&sde->flushlist_lock);
> +	spin_unlock_irqrestore(&sde->flushlist_lock, flags);
>  	queue_work_on(sde->cpu, system_highpri_wq, &sde->flush_worker);
>  	ret = -ECOMM;
>  	goto update_tail;
> -- 
> 2.17.1
>