Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch
On Tue, Jan 16, 2018 at 06:02:07PM +, 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(>ibc_list, > > > > > > 3326 > > _data.kib_reconn_list); > > 3327 else > > 3328 list_add_tail(>ibc_list, > > > > > > 3329 > > _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> --- > 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(>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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch
On Wed, Jan 17, 2018 at 12:36:19AM +, Dilger, Andreas wrote: > > > On Jan 16, 2018, at 09:56, Greg Kroah-Hartman> > wrote: > > > > On Tue, Jan 16, 2018 at 03:01:49PM +, 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 > >> --- > >> 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(>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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch
> On Jan 16, 2018, at 09:56, Greg Kroah-Hartman> wrote: > > On Tue, Jan 16, 2018 at 03:01:49PM +, 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 >> --- >> 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(>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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch
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(>ibc_list, > > > 3326 > _data.kib_reconn_list); > 3327 else > 3328 list_add_tail(>ibc_list, > > > 3329 > _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--- 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(>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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch
On Tue, Jan 16, 2018 at 03:01:49PM +, 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> --- > 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(>ibn_nconns); > } > > - kfree(conn); > + if (free_conn) > + kfree(conn); This looks really odd, don't you think? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: lustre: Fix avoid intensive reconnecting for ko2iblnd patch
In the original commit 4d99b2581effe115376402e710fbcb1c3c073769 was missed one hunk. Added it now to avoid issue with use after free. Signed-off-by: Dmitry Eremin--- 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(>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. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel