<div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial">Hi <span style="white-space: pre-wrap">Philipp,</span><div><span style="white-space: pre-wrap"><br></span></div><div>Thank you for your reply. <br></div><div>We'll get to know those patches.</div><div><br></div><div style="position:relative;zoom:1"></div><div>Best regards,</div><div>zhengbing<br></div><div><br></div><pre><br>From: Philipp Reisner <philipp.reisner@linbit.com>
Date: 2024-10-17 00:44:40
To: Dongsheng Yang <dongsheng.yang@easystack.cn>
Cc: "zhengbing.huang" <zhengbing.huang@easystack.cn>,drbd-dev@lists.linbit.com,dongsheng.yang@linux.dev
Subject: Re: [PATCH 03/11] drbd_transport_rdma: put kref for cm in dtr_path_established in error path>Hello easystack team,
>
>I probably fixed the (or some of the) problems you intended to fix
>with this patch series.
>
>Please see these recent commits on the master branch:
>
>3f82aed6e drbd_transport: fix a use after free in drbd_get_listener()
>1f2f11c47 rdma: remove dtr_path_established()
>e38120f3d rdma: Fix leaking a cm_id object
>e0f5e307e rdma: fix superficial kref_put() in error code path in
>dtr_cma_accept()
>35a6b002c rdma: fix concurrency of activate_path() and __dtr_disconnect_path()
>6ccd39432 rdma: fix an access after free() in dtr_destroy_cm()
>5a711b347 rdma: fix free() of scheduled delayed_work
>847aab659 rdma: remove misguided kref_get()/kref_put()
>
>I will go over your patch series and your comments. I'm sorry I
>dropped the ball on this. I was out on vacation in July, and when I
>came back, 1000 things needed my attention.
>
>Best regards,
> Philipp
>
>On Mon, Jul 1, 2024 at 4:48 AM Dongsheng Yang
><dongsheng.yang@easystack.cn> wrote:
>>
>>
>>
>> 在 2024/7/1 星期一 上午 10:07, Dongsheng Yang 写道:
>> >
>> >
>> > 在 2024/6/28 星期五 下午 5:40, Philipp Reisner 写道:
>> >> Hello Dongsheng,
>> >>
>> >> Please add more information why you think this change fixes a bug.
>> >> Have you experienced a leak of cm structs?
>> >> We got a RDMA_CM_EVENT_ESTABLISHED event. Even if DRBD does not do
>> >> anything with this cm, we sill expect a RDMA_CM_EVENT_DISCONNECTED in
>> >> the future. Is a problem in the handling of the disconnect?
>> >
>> > If dtr_path_established() go into this branch, it will not
>> > schedule_work(&cm->establish_work);
>> >
>> > That means path->cm->state = DSM_CONNECTED; will not be done in
>> > dtr_path_established_work_fn(), so __dtr_disconnect_path() will not call
>> > rdma_disconnect(). That means this reference will never be put.
>>
>> let me consider this example:
>> a) rdma_connect() called and RDMA_CM_EVENT_ESTABLISHED received.
>>
>> b) network failure and dtr_path_established() go into error path.
>>
>> c) establish_work will not be scheduled.
>>
>> d) drbdadm down test will hang because cm ref is not put.
>> >>
>> >> best regards,
>> >> Philipp
>> >>
>> >> On Mon, Jun 24, 2024 at 9:28 AM zhengbing.huang
>> >> <zhengbing.huang@easystack.cn> wrote:
>> >>>
>> >>> From: Dongsheng Yang <dongsheng.yang@easystack.cn>
>> >>>
>> >>> Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
>> >>> ---
>> >>> drbd/drbd_transport_rdma.c | 1 +
>> >>> 1 file changed, 1 insertion(+)
>> >>>
>> >>> diff --git a/drbd/drbd_transport_rdma.c b/drbd/drbd_transport_rdma.c
>> >>> index cfbae0e78..eccd0c6ce 100644
>> >>> --- a/drbd/drbd_transport_rdma.c
>> >>> +++ b/drbd/drbd_transport_rdma.c
>> >>> @@ -922,6 +922,7 @@ static void dtr_path_established(struct dtr_cm *cm)
>> >>> atomic_set(&cs->active_state, PCS_INACTIVE);
>> >>> wake_up(&cs->wq);
>> >>> }
>> >>> + kref_put(&cm->kref, dtr_destroy_cm);
>> >>> return;
>> >>> }
>> >>>
>> >>> --
>> >>> 2.27.0
>> >>>
</pre></div><br>