Re:Re: [PATCH 03/11] drbd_transport_rdma: put kref for cm in dtr_path_established in error path

Hi Philipp,

Thank you for your reply.

We'll get to know those patches.



Best regards,
zhengbing




From: Philipp Reisner <[email protected]>
Date: 2024-10-17 00:44:40
To:  Dongsheng Yang <[email protected]>
Cc:  "zhengbing.huang" <[email protected]>,[email protected],[email protected]
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
><[email protected]> 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
>> >> <[email protected]> wrote:
>> >>>
>> >>> From: Dongsheng Yang <[email protected]>
>> >>>
>> >>> Signed-off-by: Dongsheng Yang <[email protected]>
>> >>> ---
>> >>>   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
>> >>>