Re: [PATCH net] net/smc: fix potential sleeping issue in smc_switch_conns

[Wenjia Zhang <wenjia@linux.ibm.com>]

On 17.04.24 09:32, Guangguan Wang wrote:
> 
> 
> 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
> Agree! Thank you, Guangguan, for the examples!
If we can verify that this bug exits, we should find other solutions.
>>
>> 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
>>
>