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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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