[PATCH RESEND 0/2] Fixing libfc memory leaks

[PATCH RESEND 0/2] Fixing libfc memory leaks

[Igor Druzhinin]

Hi Hannes,

Could you take a look at those?
At least the first one causes noticeable memory decline over time
for some of our deployments.

Igor Druzhinin (2):
  scsi: libfc: free response frame from GPN_ID
  scsi: libfc: drop extra rport reference in fc_rport_create()

 drivers/scsi/libfc/fc_disc.c  | 2 ++
 drivers/scsi/libfc/fc_rport.c | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.7.4

[PATCH RESEND 1/2] scsi: libfc: free response frame from GPN_ID

[Igor Druzhinin]

fc_disc_gpn_id_resp() should be the last function using it so free it
here to avoid memory leak.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 drivers/scsi/libfc/fc_disc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
index 9c5f7c9..2b865c6 100644
--- a/drivers/scsi/libfc/fc_disc.c
+++ b/drivers/scsi/libfc/fc_disc.c
@@ -628,6 +628,8 @@ static void fc_disc_gpn_id_resp(struct fc_seq *sp, struct fc_frame *fp,
 	}
 out:
 	kref_put(&rdata->kref, fc_rport_destroy);
+	if (!IS_ERR(fp))
+		fc_frame_free(fp);
 }
 
 /**
-- 
2.7.4

[PATCH RESEND 2/2] scsi: libfc: drop extra rport reference in fc_rport_create()

[Igor Druzhinin]

The callers of this function seem to assume the reference is not taken
in case rport already exists. This results in one extra reference taken
on each rport re-discovery that will eventually get to inability to
free rport structure on port removal.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 drivers/scsi/libfc/fc_rport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index da6e97d..a43f9dd 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -133,8 +133,10 @@ struct fc_rport_priv *fc_rport_create(struct fc_lport *lport, u32 port_id)
 	lockdep_assert_held(&lport->disc.disc_mutex);
 
 	rdata = fc_rport_lookup(lport, port_id);
-	if (rdata)
+	if (rdata) {
+		kref_put(&rdata->kref, fc_rport_destroy);
 		return rdata;
+	}
 
 	if (lport->rport_priv_size > 0)
 		rport_priv_size = lport->rport_priv_size;
-- 
2.7.4

Re: [PATCH RESEND 0/2] Fixing libfc memory leaks

[Martin K. Petersen]

Hannes,

> Could you take a look at those?
> At least the first one causes noticeable memory decline over time
> for some of our deployments.
>
> Igor Druzhinin (2):
>   scsi: libfc: free response frame from GPN_ID
>   scsi: libfc: drop extra rport reference in fc_rport_create()
>
>  drivers/scsi/libfc/fc_disc.c  | 2 ++
>  drivers/scsi/libfc/fc_rport.c | 4 +++-
>  2 files changed, 5 insertions(+), 1 deletion(-)

Please review!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH RESEND 1/2] scsi: libfc: free response frame from GPN_ID

[Hannes Reinecke]

On 1/14/20 3:43 PM, Igor Druzhinin wrote:
> fc_disc_gpn_id_resp() should be the last function using it so free it
> here to avoid memory leak.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  drivers/scsi/libfc/fc_disc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
> index 9c5f7c9..2b865c6 100644
> --- a/drivers/scsi/libfc/fc_disc.c
> +++ b/drivers/scsi/libfc/fc_disc.c
> @@ -628,6 +628,8 @@ static void fc_disc_gpn_id_resp(struct fc_seq *sp, struct fc_frame *fp,
>  	}
>  out:
>  	kref_put(&rdata->kref, fc_rport_destroy);
> +	if (!IS_ERR(fp))
> +		fc_frame_free(fp);
>  }
>  
>  /**
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH RESEND 2/2] scsi: libfc: drop extra rport reference in fc_rport_create()

[Hannes Reinecke]

On 1/14/20 3:43 PM, Igor Druzhinin wrote:
> The callers of this function seem to assume the reference is not taken
> in case rport already exists. This results in one extra reference taken
> on each rport re-discovery that will eventually get to inability to
> free rport structure on port removal.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
>  drivers/scsi/libfc/fc_rport.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
> index da6e97d..a43f9dd 100644
> --- a/drivers/scsi/libfc/fc_rport.c
> +++ b/drivers/scsi/libfc/fc_rport.c
> @@ -133,8 +133,10 @@ struct fc_rport_priv *fc_rport_create(struct fc_lport *lport, u32 port_id)
>  	lockdep_assert_held(&lport->disc.disc_mutex);
>  
>  	rdata = fc_rport_lookup(lport, port_id);
> -	if (rdata)
> +	if (rdata) {
> +		kref_put(&rdata->kref, fc_rport_destroy);
>  		return rdata;
> +	}
>  
>  	if (lport->rport_priv_size > 0)
>  		rport_priv_size = lport->rport_priv_size;
> 
NAK.
The caller _does_ assume that a reference is taken once
fc_rport_create() returns non-NULL.
And the caller is responsible to drop the reference once 'rdatat' isn't
used anymore.
Any other usage is an error, but should be fixed in the caller, not here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

Re: [PATCH RESEND 1/2] scsi: libfc: free response frame from GPN_ID

[Martin K. Petersen]

Igor,

> fc_disc_gpn_id_resp() should be the last function using it so free it
> here to avoid memory leak.

Applied to 5.6/scsi-fixes, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering