Re: [PATCH net 5/5] net/smc: put sk reference if close work was canceled

Wenjia Zhang
2023-10-19T17:41:10Z

On 19.10.23 09:33, D. Wythe wrote:
>
>
> On 10/19/23 4:26 AM, Wenjia Zhang wrote:
>>
>>
>> On 17.10.23 04:06, D. Wythe wrote:
>>>
>>>
>>> On 10/13/23 3:04 AM, Wenjia Zhang wrote:
>>>>
>>>>
>>>> On 11.10.23 09:33, D. Wythe wrote:
>>>>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>>>>
>>>>> Note that we always hold a reference to sock when attempting
>>>>> to submit close_work.
>>>> yes
>>>> Therefore, if we have successfully
>>>>> canceled close_work from pending, we MUST release that reference
>>>>> to avoid potential leaks.
>>>>>
>>>> Isn't the corresponding reference already released inside the
>>>> smc_close_passive_work()?
>>>>
>>>
>>> Hi Wenjia,
>>>
>>> If we successfully cancel the close work from the pending state,
>>> it means that smc_close_passive_work() has never been executed.
>>>
>>> You can find more details here.
>>>
>>> /**
>>> * cancel_work_sync - cancel a work and wait for it to finish
>>> * @work:the work to cancel
>>> *
>>> * Cancel @work and wait for its execution to finish. This function
>>> * can be used even if the work re-queues itself or migrates to
>>> * another workqueue. On return from this function, @work is
>>> * guaranteed to be not pending or executing on any CPU.
>>> *
>>> * cancel_work_sync(&delayed_work->work) must not be used for
>>> * delayed_work's. Use cancel_delayed_work_sync() instead.
>>> *
>>> * The caller must ensure that the workqueue on which @work was last
>>> * queued can't be destroyed before this function returns.
>>> *
>>> * Return:
>>> * %true if @work was pending, %false otherwise.
>>> */
>>> boolcancel_work_sync(structwork_struct *work)
>>> {
>>> return__cancel_work_timer(work, false);
>>> }
>>>
>>> Best wishes,
>>> D. Wythe
>> As I understand, queue_work() would wake up the work if the work is
>> not already on the queue. And the sock_hold() is just prio to the
>> queue_work(). That means, cancel_work_sync() would cancel the work
>> either before its execution or after. If your fix refers to the former
>> case, at this moment, I don't think the reference can be hold, thus it
>> is unnecessary to put it.
>>>
>
> I am quite confuse about why you think when we cancel the work before
> its execution,
> the reference can not be hold ?
>
>
> Perhaps the following diagram can describe the problem in better way :
>
> smc_close_cancel_work
> smc_cdc_msg_recv_action
>
>
> sock_hold
> queue_work
>                                                                if
> (cancel_work_sync())        // successfully cancel before execution
> sock_put()                        //  need to put it since we already
> hold a ref before   queue_work()
>
>
ha, I already thought you might ask such question:P

I think here two Problems need to be clarified:

1) Do you think the bh_lock_sock/bh_unlock_sock in the smc_cdc_msg_recv
does not protect the smc_cdc_msg_recv_action() from cancel_work_sync()?
Maybe that would go back to the discussion in the other patch on the
behaviors of the locks.

2) If the queue_work returns true, as I said in the last main, the work
should be (being) executed. How could the cancel_work_sync() cancel the
work before execution successgully?

>>>>> Fixes: 42bfba9eaa33 ("net/smc: immediate termination for SMCD link
>>>>> groups")
>>>>> Signed-off-by: D. Wythe <alibuda@linux.alibaba.com>
>>>>> ---
>>>>>   net/smc/smc_close.c | 3 ++-
>>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/smc/smc_close.c b/net/smc/smc_close.c
>>>>> index 449ef45..10219f5 100644
>>>>> --- a/net/smc/smc_close.c
>>>>> +++ b/net/smc/smc_close.c
>>>>> @@ -116,7 +116,8 @@ static void smc_close_cancel_work(struct
>>>>> smc_sock *smc)
>>>>>       struct sock *sk = &smc->sk;
>>>>>         release_sock(sk);
>>>>> -    cancel_work_sync(&smc->conn.close_work);
>>>>> +    if (cancel_work_sync(&smc->conn.close_work))
>>>>> +        sock_put(sk);
>>>>>       cancel_delayed_work_sync(&smc->conn.tx_work);
>>>>>       lock_sock(sk);
>>>>>   }
>>>
>
>