[PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[Eremin, Dmitry]

In the original commit 4d99b2581effe115376402e710fbcb1c3c073769
was missed one hunk. Added it now to avoid issue with use after free.

Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
---
 drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
index 2ebc484..a15a625 100644
--- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
+++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
@@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
 		atomic_dec(&net->ibn_nconns);
 	}
 
-	kfree(conn);
+	if (free_conn)
+		kfree(conn);
 }
 
 int kiblnd_close_peer_conns_locked(struct kib_peer *peer, int why)
-- 
1.8.3.1


--------------------------------------------------------------------
Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park,
17 Krylatskaya Str., Bldg 4, Moscow 121614,
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[Greg Kroah-Hartman]

On Tue, Jan 16, 2018 at 03:01:49PM +0000, Eremin, Dmitry wrote:
> In the original commit 4d99b2581effe115376402e710fbcb1c3c073769

Please use the documented way to write this:
	4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")

> was missed one hunk. Added it now to avoid issue with use after free.

And I do not understand this commit message at all.

> 
> Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
> ---
>  drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> index 2ebc484..a15a625 100644
> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> @@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
>  		atomic_dec(&net->ibn_nconns);
>  	}
>  
> -	kfree(conn);
> +	if (free_conn)
> +		kfree(conn);

This looks really odd, don't you think?

thanks,

greg k-h

[PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[Eremin, Dmitry]

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,
>                                                                        ^^^^^^^^^^^^
>   3326                                                &kiblnd_data.kib_reconn_list);
>   3327                          else
>   3328                                  list_add_tail(&conn->ibc_list,
>                                                                         ^^^^^^^^^^^^
>   3329                                                &kiblnd_data.kib_reconn_wait);

After attached patch this code will use struct kib_conn only when it was not freed.

Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
---
 drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
index 2ebc484..a15a625 100644
--- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
+++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
@@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
 		atomic_dec(&net->ibn_nconns);
 	}
 
-	kfree(conn);
+	if (free_conn)
+		kfree(conn);
 }
 
 int kiblnd_close_peer_conns_locked(struct kib_peer *peer, int why)
-- 
1.8.3.1


--------------------------------------------------------------------
Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park,
17 Krylatskaya Str., Bldg 4, Moscow 121614,
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[Dilger, Andreas]

> On Jan 16, 2018, at 09:56, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> On Tue, Jan 16, 2018 at 03:01:49PM +0000, Eremin, Dmitry wrote:
>> In the original commit 4d99b2581effe115376402e710fbcb1c3c073769
> 
> Please use the documented way to write this:
> 	4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
> 

>> was missed one hunk. Added it now to avoid issue with use after free.
> 
> And I do not understand this commit message at all.
> 
>> Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
>> ---
>> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> index 2ebc484..a15a625 100644
>> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
>> @@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
>> 		atomic_dec(&net->ibn_nconns);
>> 	}
>> 
>> -	kfree(conn);
>> +	if (free_conn)
>> +		kfree(conn);
> 
> This looks really odd, don't you think?

I'm not sure what the objection is here?  There is an argument to this
this function named "free_conn" which determines if the structure should
be freed, or if the network connection is just being torn down and
reconnected.

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation

Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[Greg Kroah-Hartman]

On Wed, Jan 17, 2018 at 12:36:19AM +0000, Dilger, Andreas wrote:
> 
> > On Jan 16, 2018, at 09:56, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > On Tue, Jan 16, 2018 at 03:01:49PM +0000, Eremin, Dmitry wrote:
> >> In the original commit 4d99b2581effe115376402e710fbcb1c3c073769
> > 
> > Please use the documented way to write this:
> > 	4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
> > 
> 
> >> was missed one hunk. Added it now to avoid issue with use after free.
> > 
> > And I do not understand this commit message at all.
> > 
> >> Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
> >> ---
> >> drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> >> index 2ebc484..a15a625 100644
> >> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> >> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> >> @@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
> >> 		atomic_dec(&net->ibn_nconns);
> >> 	}
> >> 
> >> -	kfree(conn);
> >> +	if (free_conn)
> >> +		kfree(conn);
> > 
> > This looks really odd, don't you think?
> 
> I'm not sure what the objection is here?  There is an argument to this
> this function named "free_conn" which determines if the structure should
> be freed, or if the network connection is just being torn down and
> reconnected.

At first glance it really looks like the normal pattern of:
	if (foo_ptr)
		kfree(foo_ptr);

right?

If you don't want to free the variable, set it to NULL.

Even then, this is a horrible function, you should have 2 different
ones:
	kiblnd_destroy_conn(...)
	kiblnd_free_conn()

and then just free the variable in the free_conn() function if you were
going to set the free_conn variable, right?

That way no odd code paths are taken, and it's obvious what you are
doing just by reading the code at the callsite.

thanks,

greg k-h

Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[Greg Kroah-Hartman]

On Tue, Jan 16, 2018 at 06:02:07PM +0000, Eremin, Dmitry 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,
> >                                                                        ^^^^^^^^^^^^
> >   3326                                                &kiblnd_data.kib_reconn_list);
> >   3327                          else
> >   3328                                  list_add_tail(&conn->ibc_list,
> >                                                                         ^^^^^^^^^^^^
> >   3329                                                &kiblnd_data.kib_reconn_wait);
> 
> After attached patch this code will use struct kib_conn only when it was not freed.
> 
> Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
> ---
>  drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> index 2ebc484..a15a625 100644
> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> @@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
>  		atomic_dec(&net->ibn_nconns);
>  	}
>  
> -	kfree(conn);
> +	if (free_conn)
> +		kfree(conn);
>  }
>  
>  int kiblnd_close_peer_conns_locked(struct kib_peer *peer, int why)

This patch needs a real "Fixes:" tag, right?

Also, as this was from 4.6, it should go to the stable trees, right?
Can you resend this with that info, and then send a follow-on patch to
fix this up the way I recommended so that no one is confused in the
future?

thanks,

greg k-h

[PATCH v2] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch

[Dmitry Eremin]

From: Dmitry Eremin <Dmitry.Eremin@intel.com>

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

After attached patch this code will use struct kib_conn only when it was not freed.

Cc: <stable@vger.kernel.org> # v4.6
Fixes: 4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
---
 drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
index 2ebc484385b3..a15a625ee9b6 100644
--- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
+++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
@@ -890,7 +890,8 @@ void kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn)
 		atomic_dec(&net->ibn_nconns);
 	}
 
-	kfree(conn);
+	if (free_conn)
+		kfree(conn);
 }
 
 int kiblnd_close_peer_conns_locked(struct kib_peer *peer, int why)
-- 
1.8.3.1


--------------------------------------------------------------------
Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park,
17 Krylatskaya Str., Bldg 4, Moscow 121614,
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3] staging: lustre: separate a connection destroy from free struct kib_conn

[Dmitry Eremin]

Rewrite the logic of the original commit 4d99b2581eff ("staging: lustre: avoid
intensive reconnecting for ko2iblnd") and move the freeing a struct kib_conn
outside of the function kiblnd_destroy_conn(). The freeing of struct kib_conn
should be depending on the second argument free_conn of function
kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn). If it true the
structure should be freed.

Fixes: 4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
---
 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


--------------------------------------------------------------------
Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park,
17 Krylatskaya Str., Bldg 4, Moscow 121614,
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: lustre: separate a connection destroy from free struct kib_conn

[Greg Kroah-Hartman]

On Wed, Jan 24, 2018 at 05:14:12PM +0300, Dmitry Eremin wrote:
> Rewrite the logic of the original commit 4d99b2581eff ("staging: lustre: avoid
> intensive reconnecting for ko2iblnd") and move the freeing a struct kib_conn
> outside of the function kiblnd_destroy_conn(). The freeing of struct kib_conn
> should be depending on the second argument free_conn of function
> kiblnd_destroy_conn(struct kib_conn *conn, bool free_conn). If it true the
> structure should be freed.
> 
> Fixes: 4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
> Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
> ---
>  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(-)

What changed from version 2 and 1?  Always included that below the ---
line please.

4th try is a charm?  :)

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v4] staging: lustre: separate a connection destroy from free struct kib_conn

[Dmitry Eremin]

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@vger.kernel.org> # v4.6
Fixes: 4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
---
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


--------------------------------------------------------------------
Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park,
17 Krylatskaya Str., Bldg 4, Moscow 121614,
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v4] staging: lustre: separate a connection destroy from free struct kib_conn

[Dilger, Andreas]

On Jan 25, 2018, at 06:51, Eremin, Dmitry <dmitry.eremin@intel.com> 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@vger.kernel.org> # v4.6
> Fixes: 4d99b2581eff ("staging: lustre: avoid intensive reconnecting for ko2iblnd")
> Signed-off-by: Dmitry Eremin <Dmitry.Eremin@intel.com>

Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>

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