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

["D. Wythe" <alibuda@linux.alibaba.com>]

On 10/20/23 1:40 AM, Wenjia Zhang wrote:
>
>
> 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.
>

Yes. bh_lock_sock/bh_unlock_sock can not block code execution protected 
by lock_sock/unlock(). That is to say, they are not exclusive.

We can use a very simple example to infer that since bh_lock_sock is 
type of spin-lock, if bh_lock_sock/bh_unlock_sock can block 
lock_sock/unlock(),
then lock_sock/unlock() can also block bh_lock_sock/bh_unlock_sock.

If this is true, when the process context already lock_sock(), the 
interrupt context must wait for the process to call
release_sock(). Obviously, this is very unreasonable.


> 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?

No, that's not true. In fact, if queue_work returns true, it simply 
means that we have added the task to the queue and may schedule a worker 
to execute it,
but it does not guarantee that the task will be executed or is being 
executed when it returns true,
the task might still in the list and waiting some worker to execute it.

We can make a simple inference,

1. A known fact is that if no special flag (WORK_UNBOUND) is given, 
tasks submitted will eventually be executed on the CPU where they were 
submitted.

2. If the queue_work returns true, the work should be or is being executed

If all of the above are true, when we invoke queue_work in an interrupt 
context, does it mean that the submitted task will be executed in the 
interrupt context?


Best wishes,
D. Wythe

>
>>>>>> 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);
>>>>>>   }
>>>>
>>
>>