[PATCH] libfc: fix mem leak in fc_seq_assign()

[PATCH] libfc: fix mem leak in fc_seq_assign()

[Hillf Danton]

There seems exchs should get released in cases that FC_RJT_NONE is
returned, or they will no longer get freed.

It also looks a typo like the following,

-		    fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
+		    fc_seq_lookup_recip(lport, ema->mp, fp) == FC_RJT_NONE)

but I am not sure.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/drivers/scsi/libfc/fc_exch.c	2010-09-13 07:07:38.000000000 +0800
+++ b/drivers/scsi/libfc/fc_exch.c	2010-10-19 21:48:22.000000000 +0800
@@ -3,6 +3,9 @@
  * Copyright(c) 2008 Red Hat, Inc.  All rights reserved.
  * Copyright(c) 2008 Mike Christie
  *
+ * Copyright(c) Oct 19, 2010	Hillf Danton
+ *				minor fix of memory leakage
+ *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
  * version 2, as published by the Free Software Foundation.
@@ -1246,9 +1249,16 @@ static struct fc_seq *fc_seq_assign(stru
 	fr_seq(fp) = NULL;

 	list_for_each_entry(ema, &lport->ema_list, ema_list)
-		if ((!ema->match || ema->match(fp)) &&
-		    fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
-			break;
+		if (! ema->match || ema->match(fp))
+			if (fc_seq_lookup_recip(lport, ema->mp, fp) !=
+				FC_RJT_NONE)
+				break;
+			else {
+				struct fc_seq *seq = fr_seq(fp);
+				struct fc_exch *exch = fc_seq_exch(seq);
+				fc_exch_release(exch);
+			}
+
 	return fr_seq(fp);
 }

RE: [PATCH] libfc: fix mem leak in fc_seq_assign()

[Zou, Yi]

> 
> There seems exchs should get released in cases that FC_RJT_NONE is
> returned, or they will no longer get freed.
> 
> It also looks a typo like the following,
> 
> -		    fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
> +		    fc_seq_lookup_recip(lport, ema->mp, fp) == FC_RJT_NONE)
> 
> but I am not sure.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/drivers/scsi/libfc/fc_exch.c	2010-09-13 07:07:38.000000000 +0800
> +++ b/drivers/scsi/libfc/fc_exch.c	2010-10-19 21:48:22.000000000 +0800
> @@ -3,6 +3,9 @@
>   * Copyright(c) 2008 Red Hat, Inc.  All rights reserved.
>   * Copyright(c) 2008 Mike Christie
>   *
> + * Copyright(c) Oct 19, 2010	Hillf Danton
> + *				minor fix of memory leakage
> + *
>   * This program is free software; you can redistribute it and/or modify
> it
>   * under the terms and conditions of the GNU General Public License,
>   * version 2, as published by the Free Software Foundation.
> @@ -1246,9 +1249,16 @@ static struct fc_seq *fc_seq_assign(stru
>  	fr_seq(fp) = NULL;
> 
>  	list_for_each_entry(ema, &lport->ema_list, ema_list)
> -		if ((!ema->match || ema->match(fp)) &&
> -		    fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
> -			break;
> +		if (! ema->match || ema->match(fp))
> +			if (fc_seq_lookup_recip(lport, ema->mp, fp) !=
> +				FC_RJT_NONE)
> +				break;
This looks more to be a '==' here as you pointed out since you either 
get a rejection code or have found/allocated exchange via the lookup_recip()
here. When not RJT_NONE, try next em in the list, however, does not look
like anywhere the RJT is sent back when not RJT_NONE. Probably the caller
would check if the return being NULL then send its own rejection code.

> +			else {
> +				struct fc_seq *seq = fr_seq(fp);
> +				struct fc_exch *exch = fc_seq_exch(seq);
> +				fc_exch_release(exch);
> +			}
> +
I believe you want to hold the exch here if you find one.

>  	return fr_seq(fp);
>  }

Re: [PATCH] libfc: fix mem leak in fc_seq_assign()

[Joe Eykholt]

On 10/19/10 7:17 AM, Hillf Danton wrote:
> There seems exchs should get released in cases that FC_RJT_NONE is
> returned, or they will no longer get freed.
> 
> It also looks a typo like the following,
> 
> -		    fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
> +		    fc_seq_lookup_recip(lport, ema->mp, fp) == FC_RJT_NONE)

True.

> 
> but I am not sure.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/drivers/scsi/libfc/fc_exch.c	2010-09-13 07:07:38.000000000 +0800
> +++ b/drivers/scsi/libfc/fc_exch.c	2010-10-19 21:48:22.000000000 +0800
> @@ -3,6 +3,9 @@
>   * Copyright(c) 2008 Red Hat, Inc.  All rights reserved.
>   * Copyright(c) 2008 Mike Christie
>   *
> + * Copyright(c) Oct 19, 2010	Hillf Danton
> + *				minor fix of memory leakage
> + *

Thanks for fixing the bug, but I don't think you should add a copyright.
I didn't add a copyright when I created the bug.

>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
>   * version 2, as published by the Free Software Foundation.
> @@ -1246,9 +1249,16 @@ static struct fc_seq *fc_seq_assign(stru
>  	fr_seq(fp) = NULL;
> 
>  	list_for_each_entry(ema, &lport->ema_list, ema_list)
> -		if ((!ema->match || ema->match(fp)) &&
> -		    fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
> -			break;
> +		if (! ema->match || ema->match(fp))
> +			if (fc_seq_lookup_recip(lport, ema->mp, fp) !=
> +				FC_RJT_NONE)

Should continue in this case ... we hope to find the exchange in another
exchange manager.

> +				break;
> +			else {
> +				struct fc_seq *seq = fr_seq(fp);
> +				struct fc_exch *exch = fc_seq_exch(seq);
> +				fc_exch_release(exch);

I want the sequence to be held by the frame, so don't do the release.
In this case we should break, since we found the frame.

This function is currently only used by target modules.  A later
patch exposes a release so they can release the sequence/exchange.

So, my original code was right except for the != vs == bug.
That turns out not to cause major problems if the first exchange
manager assigns the exchange, which is why I didn't see the bug.

> +			}
> +
>  	return fr_seq(fp);
>  }

By the way, I think patches to libfc, libfcoe, and fcoe should go to
the open-fcoe list only, not to SCSI.  Robert Love as the open-fcoe
maintainer will forward them after testing to the SCSI list.

	Regards,
	Joe

Re: [PATCH] libfc: fix mem leak in fc_seq_assign()

[Hillf Danton]

On Wed, Oct 20, 2010 at 1:29 AM, Joe Eykholt <jeykholt@cisco.com> wrote:
>
> On 10/19/10 7:17 AM, Hillf Danton wrote:
>> There seems exchs should get released in cases that FC_RJT_NONE is
>> returned, or they will no longer get freed.
>>
>> It also looks a typo like the following,
>>
>> -                 fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
>> +                 fc_seq_lookup_recip(lport, ema->mp, fp) == FC_RJT_NONE)
>
> True.
>
>>
>> but I am not sure.
>>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>>
>> --- a/drivers/scsi/libfc/fc_exch.c    2010-09-13 07:07:38.000000000 +0800
>> +++ b/drivers/scsi/libfc/fc_exch.c    2010-10-19 21:48:22.000000000 +0800
>> @@ -3,6 +3,9 @@
>>   * Copyright(c) 2008 Red Hat, Inc.  All rights reserved.
>>   * Copyright(c) 2008 Mike Christie
>>   *
>> + * Copyright(c) Oct 19, 2010 Hillf Danton
>> + *                           minor fix of memory leakage
>> + *
>
> Thanks for fixing the bug, but I don't think you should add a copyright.

Node

> I didn't add a copyright when I created the bug.

The cool comment I ever seen, haha.

And the true patch will be prepared and delivered soon.

thanks //Hillf

>>   * This program is free software; you can redistribute it and/or modify it
>>   * under the terms and conditions of the GNU General Public License,
>>   * version 2, as published by the Free Software Foundation.
>> @@ -1246,9 +1249,16 @@ static struct fc_seq *fc_seq_assign(stru
>>       fr_seq(fp) = NULL;
>>
>>       list_for_each_entry(ema, &lport->ema_list, ema_list)
>> -             if ((!ema->match || ema->match(fp)) &&
>> -                 fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
>> -                     break;
>> +             if (! ema->match || ema->match(fp))
>> +                     if (fc_seq_lookup_recip(lport, ema->mp, fp) !=
>> +                             FC_RJT_NONE)
>
> Should continue in this case ... we hope to find the exchange in another
> exchange manager.
>
>> +                             break;
>> +                     else {
>> +                             struct fc_seq *seq = fr_seq(fp);
>> +                             struct fc_exch *exch = fc_seq_exch(seq);
>> +                             fc_exch_release(exch);
>
> I want the sequence to be held by the frame, so don't do the release.
> In this case we should break, since we found the frame.
>
> This function is currently only used by target modules.  A later
> patch exposes a release so they can release the sequence/exchange.
>
> So, my original code was right except for the != vs == bug.
> That turns out not to cause major problems if the first exchange
> manager assigns the exchange, which is why I didn't see the bug.
>
>> +                     }
>> +
>>       return fr_seq(fp);
>>  }
>
> By the way, I think patches to libfc, libfcoe, and fcoe should go to
> the open-fcoe list only, not to SCSI.  Robert Love as the open-fcoe
> maintainer will forward them after testing to the SCSI list.
>
>        Regards,
>        Joe
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libfc: fix mem leak in fc_seq_assign()

[Hillf Danton]

There is a typo cleaned, which triggers memory leakage.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/drivers/scsi/libfc/fc_exch.c	2010-09-13 07:07:38.000000000 +0800
+++ b/drivers/scsi/libfc/fc_exch.c	2010-10-23 15:00:08.000000000 +0800
@@ -3,6 +3,9 @@
  * Copyright(c) 2008 Red Hat, Inc.  All rights reserved.
  * Copyright(c) 2008 Mike Christie
  *
+ * Oct 23 2010		Hillf Danton
+ *			minor fix of memory leakage
+ *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
  * version 2, as published by the Free Software Foundation.
@@ -1247,7 +1250,7 @@ static struct fc_seq *fc_seq_assign(stru

 	list_for_each_entry(ema, &lport->ema_list, ema_list)
 		if ((!ema->match || ema->match(fp)) &&
-		    fc_seq_lookup_recip(lport, ema->mp, fp) != FC_RJT_NONE)
+		    fc_seq_lookup_recip(lport, ema->mp, fp) == FC_RJT_NONE)
 			break;
 	return fr_seq(fp);
 }