netdev - Re: [PATCH net] net/smc: fix potential sleeping issue in smc_switch

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: <a94de96f-8b18-482c-90e2-7f8584528bc8@linux.alibaba.com>
Date: Wed, 17 Apr 2024 15:32:05 +0800
From: Guangguan Wang <guangguan.wang@...ux.alibaba.com>
To: Paolo Abeni <pabeni@...hat.com>, Zhengchao Shao <shaozhengchao@...wei.com>
Cc: wenjia@...ux.ibm.com, jaka@...ux.ibm.com, alibuda@...ux.alibaba.com,
 tonylu@...ux.alibaba.com, guwen@...ux.alibaba.com, weiyongjun1@...wei.com,
 yuehaibing@...wei.com, tangchengchang@...wei.com, kuba@...nel.org,
 edumazet@...gle.com, davem@...emloft.net, netdev@...r.kernel.org,
 linux-s390@...r.kernel.org
Subject: Re: [PATCH net] net/smc: fix potential sleeping issue in
 smc_switch_conns



On 2024/4/16 20:06, Paolo Abeni wrote:
> On Sat, 2024-04-13 at 11:51 +0800, Zhengchao Shao wrote:
>> Potential sleeping issue exists in the following processes:
>> smc_switch_conns
>>   spin_lock_bh(&conn->send_lock)
>>   smc_switch_link_and_count
>>     smcr_link_put
>>       __smcr_link_clear
>>         smc_lgr_put
>>           __smc_lgr_free
>>             smc_lgr_free_bufs
>>               __smc_lgr_free_bufs
>>                 smc_buf_free
>>                   smcr_buf_free
>>                     smcr_buf_unmap_link
>>                       smc_ib_put_memory_region
>>                         ib_dereg_mr
>>                           ib_dereg_mr_user
>>                             mr->device->ops.dereg_mr
>> If scheduling exists when the IB driver implements .dereg_mr hook
>> function, the bug "scheduling while atomic" will occur. For example,
>> cxgb4 and efa driver. Use mutex lock instead of spin lock to fix it.
> 
> I tried to inspect all the lock call sites, and it *look* like they are
> all in process context, so the switch should be feasible.

There exist some calls from tasklet, where mutex lock is infeasible.
For example:
- tasklet -> smc_wr_tx_tasklet_fn -> smc_wr_tx_process_cqe -> pnd_snd.handler -> smc_cdc_tx_handler -> smc_tx_pending -> smc_tx_sndbuf_nonempty -> smcr_tx_sndbuf_nonempty -> spin_lock_bh(&conn->send_lock)
- tasklet -> smc_wr_rx_tasklet_fn -> smc_wr_rx_process_cqes -> smc_wr_rx_demultiplex -> smc_cdc_rx_handler -> smc_cdc_msg_validate -> spin_lock_bh(&conn->send_lock)

Thanks,
Guangguan Wang

> 
> Still the fact that the existing lock is a BH variant is suspect.
> Either the BH part was not needed or this can introduce subtle
> regressions/issues. 
> 
> I think this deserves at least a 3rd party testing.
> 
> Thanks,
> 
> Paolo
>