On Jan 25, 2018, at 06:51, Eremin, Dmitry <dmitry.eremin@xxxxxxxxx> wrote:
>
> The logic of the original commit 4d99b2581eff ("staging: lustre: avoid
> intensive reconnecting for ko2iblnd") was assumed conditional free of
> struct kib_conn if the second argument free_conn in function
> kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn) is true.
> But this hunk of code was dropped from original commit. As result the logic
> works wrong and current code use struct kib_conn after free.
>
>> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
>> 3317 kiblnd_destroy_conn(conn, !peer);
>> ^^^^ Freed always (but should be conditionally)
>> 3318
>> 3319 spin_lock_irqsave(lock, flags);
>> 3320 if (!peer)
>> 3321 continue;
>> 3322
>> 3323 conn->ibc_peer = peer;
>> ^^^^^^^^^^^^^^ Use after free
>> 3324 if (peer->ibp_reconnected < KIB_RECONN_HIGH_RACE)
>> 3325 list_add_tail(&conn->ibc_list,
>> ^^^^^^^^^^^^^^ Use after free
>> 3326 &kiblnd_data.kib_reconn_list);
>> 3327 else
>> 3328 list_add_tail(&conn->ibc_list,
>> ^^^^^^^^^^^^^^ Use after free
>> 3329 &kiblnd_data.kib_reconn_wait);
>
> To avoid confusion this fix moved the freeing a struct kib_conn outside of
> the function kiblnd_destroy_conn() and free as it was intended in original
> commit.
>
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.6
> Fixes: 4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
> Signed-off-by: Dmitry Eremin <Dmitry.Eremin@xxxxxxxxx>
Reviewed-by: Andreas Dilger <andreas.dilger@xxxxxxxxx>
> ---
> Changes in v4:
> - fixed the issue with use after free by moving the freeing a struct
> kib_conn outside of the function kiblnd_destroy_conn()
>
> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 7 +++----
> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h | 2 +-
> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 6 ++++--
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> index 2ebc484385b3..ec84edfda271 100644
> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> @@ -824,14 +824,15 @@ struct kib_conn *kiblnd_create_conn(struct kib_peer *peer, struct rdma_cm_id *cm
> return conn;
>
> failed_2:
> - kiblnd_destroy_conn(conn, true);
> + kiblnd_destroy_conn(conn);
> + kfree(conn);
> failed_1:
> kfree(init_qp_attr);
> failed_0:
> return NULL;
> }
>
> -void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
> +void kiblnd_destroy_conn(struct kib_conn *conn)
> {
> struct rdma_cm_id *cmid = conn->ibc_cmid;
> struct kib_peer *peer = conn->ibc_peer;
> @@ -889,8 +890,6 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
> rdma_destroy_id(cmid);
> atomic_dec(&net->ibn_nconns);
> }
> -
> - kfree(conn);
> }
>
> int kiblnd_close_peer_conns_locked(struct kib_peer *peer, int why)
> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
> index 171eced213f8..b18911d09e9a 100644
> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.h
> @@ -1016,7 +1016,7 @@ int kiblnd_close_stale_conns_locked(struct kib_peer *peer,
> struct kib_conn *kiblnd_create_conn(struct kib_peer *peer,
> struct rdma_cm_id *cmid,
> int state, int version);
> -void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn);
> +void kiblnd_destroy_conn(struct kib_conn *conn);
> void kiblnd_close_conn(struct kib_conn *conn, int error);
> void kiblnd_close_conn_locked(struct kib_conn *conn, int error);
>
> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> index 9b3328c5d1e7..b3e7f28eb978 100644
> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c
> @@ -3314,11 +3314,13 @@ static int kiblnd_resolve_addr(struct rdma_cm_id *cmid,
> spin_unlock_irqrestore(lock, flags);
> dropped_lock = 1;
>
> - kiblnd_destroy_conn(conn, !peer);
> + kiblnd_destroy_conn(conn);
>
> spin_lock_irqsave(lock, flags);
> - if (!peer)
> + if (!peer) {
> + kfree(conn);
> continue;
> + }
>
> conn->ibc_peer = peer;
> if (peer->ibp_reconnected < KIB_RECONN_HIGH_RACE)
> --
> 1.8.3.1
>
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation