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