diff mbox series

[2/4] ata,scsi: libata-core: Do not leak memory for ata_port struct members

Message ID 20240629124210.181537-8-cassel@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series libata/libsas cleanup fixes | expand

Commit Message

Niklas Cassel June 29, 2024, 12:42 p.m. UTC
libsas is currently not freeing all the struct ata_port struct members,
e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).

Add a function, ata_port_free(), that is used to free a ata_port,
including its struct members. It makes sense to keep the code related to
freeing a ata_port in its own function, which will also free all the
struct members of struct ata_port.

Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-core.c          | 24 ++++++++++++++----------
 drivers/scsi/libsas/sas_ata.c      |  2 +-
 drivers/scsi/libsas/sas_discover.c |  2 +-
 include/linux/libata.h             |  1 +
 4 files changed, 17 insertions(+), 12 deletions(-)

Comments

John Garry June 30, 2024, 9:42 a.m. UTC | #1
On 29/06/2024 13:42, Niklas Cassel wrote:
> libsas is currently not freeing all the struct ata_port struct members,
> e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).
> 
> Add a function, ata_port_free(), that is used to free a ata_port,
> including its struct members. It makes sense to keep the code related to
> freeing a ata_port in its own function, which will also free all the
> struct members of struct ata_port.
> 
> Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Apart from a couple of nitpicks:

Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
>   drivers/ata/libata-core.c          | 24 ++++++++++++++----------
>   drivers/scsi/libsas/sas_ata.c      |  2 +-
>   drivers/scsi/libsas/sas_discover.c |  2 +-
>   include/linux/libata.h             |  1 +
>   4 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index f47838da75d7..481baa55ebfc 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5490,6 +5490,18 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
>   	return ap;
>   }
>   
> +void ata_port_free(struct ata_port *ap)
> +{
> +	if (!ap)
> +		return;

nit: personally I'd have the caller check this. The main reason for that 
is that often it seems an unnecessary check.

> +
> +	kfree(ap->pmp_link);
> +	kfree(ap->slave_link);
> +	kfree(ap->ncq_sense_buf);
> +	kfree(ap);
> +}
> +EXPORT_SYMBOL_GPL(ata_port_free);
> +
>   static void ata_devres_release(struct device *gendev, void *res)
>   {
>   	struct ata_host *host = dev_get_drvdata(gendev);
> @@ -5516,15 +5528,7 @@ static void ata_host_release(struct kref *kref)
>   	int i;
>   
>   	for (i = 0; i < host->n_ports; i++) {
> -		struct ata_port *ap = host->ports[i];
> -
> -		if (!ap)
> -			continue;
> -
> -		kfree(ap->pmp_link);
> -		kfree(ap->slave_link);
> -		kfree(ap->ncq_sense_buf);
> -		kfree(ap);
> +		ata_port_free(host->ports[i]);
>   		host->ports[i] = NULL;
>   	}
>   	kfree(host);
> @@ -5907,7 +5911,7 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
>   	 * allocation time.
>   	 */
>   	for (i = host->n_ports; host->ports[i]; i++)
> -		kfree(host->ports[i]);
> +		ata_port_free(host->ports[i]);
>   
>   	/* give ports names and add SCSI hosts */
>   	for (i = 0; i < host->n_ports; i++) {
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 4c69fc63c119..1f247a8cd185 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -618,7 +618,7 @@ int sas_ata_init(struct domain_device *found_dev)
>   	return 0;
>   
>   destroy_port:
> -	kfree(ap);
> +	ata_port_free(ap);

nit: If not a nuisance, could we change the label name to free_port, 
like free_host, below. No big deal.

>   free_host:
>   	ata_host_put(ata_host);
>   	return rc;
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 8fb7c41c0962..48d975c6dbf2 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
>   
>   	if (dev_is_sata(dev) && dev->sata_dev.ap) {
>   		ata_sas_tport_delete(dev->sata_dev.ap);
> -		kfree(dev->sata_dev.ap);
> +		ata_port_free(dev->sata_dev.ap);
>   		ata_host_put(dev->sata_dev.ata_host);
>   		dev->sata_dev.ata_host = NULL;
>   		dev->sata_dev.ap = NULL;
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 13fb41d25da6..7d3bd7c9664a 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1249,6 +1249,7 @@ extern int ata_slave_link_init(struct ata_port *ap);
>   extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
>   					   struct ata_port_info *, struct Scsi_Host *);
>   extern void ata_port_probe(struct ata_port *ap);
> +extern void ata_port_free(struct ata_port *ap);
>   extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
>   extern void ata_sas_tport_delete(struct ata_port *ap);
>   int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,
Niklas Cassel June 30, 2024, 8:28 p.m. UTC | #2
On Sun, Jun 30, 2024 at 10:42:45AM +0100, John Garry wrote:
> On 29/06/2024 13:42, Niklas Cassel wrote:
> > libsas is currently not freeing all the struct ata_port struct members,
> > e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).
> > 
> > Add a function, ata_port_free(), that is used to free a ata_port,
> > including its struct members. It makes sense to keep the code related to
> > freeing a ata_port in its own function, which will also free all the
> > struct members of struct ata_port.
> > 
> > Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Apart from a couple of nitpicks:
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
> > ---
> >   drivers/ata/libata-core.c          | 24 ++++++++++++++----------
> >   drivers/scsi/libsas/sas_ata.c      |  2 +-
> >   drivers/scsi/libsas/sas_discover.c |  2 +-
> >   include/linux/libata.h             |  1 +
> >   4 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index f47838da75d7..481baa55ebfc 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5490,6 +5490,18 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
> >   	return ap;
> >   }
> > +void ata_port_free(struct ata_port *ap)
> > +{
> > +	if (!ap)
> > +		return;
> 
> nit: personally I'd have the caller check this. The main reason for that is
> that often it seems an unnecessary check.

People are used to free() family of functions handling NULL,
so I think it is wise to keep it as is.


> 
> > +
> > +	kfree(ap->pmp_link);
> > +	kfree(ap->slave_link);
> > +	kfree(ap->ncq_sense_buf);
> > +	kfree(ap);
> > +}
> > +EXPORT_SYMBOL_GPL(ata_port_free);
> > +
> >   static void ata_devres_release(struct device *gendev, void *res)
> >   {
> >   	struct ata_host *host = dev_get_drvdata(gendev);
> > @@ -5516,15 +5528,7 @@ static void ata_host_release(struct kref *kref)
> >   	int i;
> >   	for (i = 0; i < host->n_ports; i++) {
> > -		struct ata_port *ap = host->ports[i];
> > -
> > -		if (!ap)
> > -			continue;
> > -
> > -		kfree(ap->pmp_link);
> > -		kfree(ap->slave_link);
> > -		kfree(ap->ncq_sense_buf);
> > -		kfree(ap);
> > +		ata_port_free(host->ports[i]);
> >   		host->ports[i] = NULL;
> >   	}
> >   	kfree(host);
> > @@ -5907,7 +5911,7 @@ int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
> >   	 * allocation time.
> >   	 */
> >   	for (i = host->n_ports; host->ports[i]; i++)
> > -		kfree(host->ports[i]);
> > +		ata_port_free(host->ports[i]);
> >   	/* give ports names and add SCSI hosts */
> >   	for (i = 0; i < host->n_ports; i++) {
> > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> > index 4c69fc63c119..1f247a8cd185 100644
> > --- a/drivers/scsi/libsas/sas_ata.c
> > +++ b/drivers/scsi/libsas/sas_ata.c
> > @@ -618,7 +618,7 @@ int sas_ata_init(struct domain_device *found_dev)
> >   	return 0;
> >   destroy_port:
> > -	kfree(ap);
> > +	ata_port_free(ap);
> 
> nit: If not a nuisance, could we change the label name to free_port, like
> free_host, below. No big deal.

Sure, will fixup when applying.


> 
> >   free_host:
> >   	ata_host_put(ata_host);
> >   	return rc;
> > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> > index 8fb7c41c0962..48d975c6dbf2 100644
> > --- a/drivers/scsi/libsas/sas_discover.c
> > +++ b/drivers/scsi/libsas/sas_discover.c
> > @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref)
> >   	if (dev_is_sata(dev) && dev->sata_dev.ap) {
> >   		ata_sas_tport_delete(dev->sata_dev.ap);
> > -		kfree(dev->sata_dev.ap);
> > +		ata_port_free(dev->sata_dev.ap);
> >   		ata_host_put(dev->sata_dev.ata_host);
> >   		dev->sata_dev.ata_host = NULL;
> >   		dev->sata_dev.ap = NULL;
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 13fb41d25da6..7d3bd7c9664a 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1249,6 +1249,7 @@ extern int ata_slave_link_init(struct ata_port *ap);
> >   extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
> >   					   struct ata_port_info *, struct Scsi_Host *);
> >   extern void ata_port_probe(struct ata_port *ap);
> > +extern void ata_port_free(struct ata_port *ap);
> >   extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
> >   extern void ata_sas_tport_delete(struct ata_port *ap);
> >   int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,
>
Hannes Reinecke July 1, 2024, 6:21 a.m. UTC | #3
On 6/29/24 14:42, Niklas Cassel wrote:
> libsas is currently not freeing all the struct ata_port struct members,
> e.g. ncq_sense_buf for a driver supporting Command Duration Limits (CDL).
> 
> Add a function, ata_port_free(), that is used to free a ata_port,
> including its struct members. It makes sense to keep the code related to
> freeing a ata_port in its own function, which will also free all the
> struct members of struct ata_port.
> 
> Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-core.c          | 24 ++++++++++++++----------
>   drivers/scsi/libsas/sas_ata.c      |  2 +-
>   drivers/scsi/libsas/sas_discover.c |  2 +-
>   include/linux/libata.h             |  1 +
>   4 files changed, 17 insertions(+), 12 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
diff mbox series

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index f47838da75d7..481baa55ebfc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5490,6 +5490,18 @@  struct ata_port *ata_port_alloc(struct ata_host *host)
 	return ap;
 }
 
+void ata_port_free(struct ata_port *ap)
+{
+	if (!ap)
+		return;
+
+	kfree(ap->pmp_link);
+	kfree(ap->slave_link);
+	kfree(ap->ncq_sense_buf);
+	kfree(ap);
+}
+EXPORT_SYMBOL_GPL(ata_port_free);
+
 static void ata_devres_release(struct device *gendev, void *res)
 {
 	struct ata_host *host = dev_get_drvdata(gendev);
@@ -5516,15 +5528,7 @@  static void ata_host_release(struct kref *kref)
 	int i;
 
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
-
-		if (!ap)
-			continue;
-
-		kfree(ap->pmp_link);
-		kfree(ap->slave_link);
-		kfree(ap->ncq_sense_buf);
-		kfree(ap);
+		ata_port_free(host->ports[i]);
 		host->ports[i] = NULL;
 	}
 	kfree(host);
@@ -5907,7 +5911,7 @@  int ata_host_register(struct ata_host *host, const struct scsi_host_template *sh
 	 * allocation time.
 	 */
 	for (i = host->n_ports; host->ports[i]; i++)
-		kfree(host->ports[i]);
+		ata_port_free(host->ports[i]);
 
 	/* give ports names and add SCSI hosts */
 	for (i = 0; i < host->n_ports; i++) {
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 4c69fc63c119..1f247a8cd185 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -618,7 +618,7 @@  int sas_ata_init(struct domain_device *found_dev)
 	return 0;
 
 destroy_port:
-	kfree(ap);
+	ata_port_free(ap);
 free_host:
 	ata_host_put(ata_host);
 	return rc;
diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 8fb7c41c0962..48d975c6dbf2 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -301,7 +301,7 @@  void sas_free_device(struct kref *kref)
 
 	if (dev_is_sata(dev) && dev->sata_dev.ap) {
 		ata_sas_tport_delete(dev->sata_dev.ap);
-		kfree(dev->sata_dev.ap);
+		ata_port_free(dev->sata_dev.ap);
 		ata_host_put(dev->sata_dev.ata_host);
 		dev->sata_dev.ata_host = NULL;
 		dev->sata_dev.ap = NULL;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 13fb41d25da6..7d3bd7c9664a 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1249,6 +1249,7 @@  extern int ata_slave_link_init(struct ata_port *ap);
 extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 					   struct ata_port_info *, struct Scsi_Host *);
 extern void ata_port_probe(struct ata_port *ap);
+extern void ata_port_free(struct ata_port *ap);
 extern int ata_sas_tport_add(struct device *parent, struct ata_port *ap);
 extern void ata_sas_tport_delete(struct ata_port *ap);
 int ata_sas_device_configure(struct scsi_device *sdev, struct queue_limits *lim,