2020-07-09 12:03:36

by Jing Xiangfeng

Subject: [PATCH v2] scsi: fcoe: add missed kfree() in an error path

fcoe_fdmi_info() misses to call kfree() in an error path.
Add a label 'free_fdmi' and jump to it.

Fixes: f07d46bbc9ba ("fcoe: Fix smatch warning in fcoe_fdmi_info function")
Signed-off-by: Jing Xiangfeng <[email protected]>
---
drivers/scsi/fcoe/fcoe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 25dae9f0b205..a63057a03772 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -830,7 +830,7 @@ static void fcoe_fdmi_info(struct fc_lport *lport, struct net_device *netdev)
if (rc) {
printk(KERN_INFO "fcoe: Failed to retrieve FDMI "
"information from netdev.\n");
- return;
+ goto free_fdmi;
}

snprintf(fc_host_serial_number(lport->host),
@@ -868,6 +868,7 @@ static void fcoe_fdmi_info(struct fc_lport *lport, struct net_device *netdev)

/* Enable FDMI lport states */
lport->fdmi_enabled = 1;
+free_fdmi:
kfree(fdmi);
} else {
lport->fdmi_enabled = 0;
--
2.17.1

2020-07-13 17:54:39

by Ewan Milne

Subject: Re: [PATCH v2] scsi: fcoe: add missed kfree() in an error path

See below.

On Thu, 2020-07-09 at 20:05 +0800, Jing Xiangfeng wrote:
> fcoe_fdmi_info() misses to call kfree() in an error path.
> Add a label 'free_fdmi' and jump to it.
>
> Fixes: f07d46bbc9ba ("fcoe: Fix smatch warning in fcoe_fdmi_info
> function")
> Signed-off-by: Jing Xiangfeng <[email protected]>
> ---
> drivers/scsi/fcoe/fcoe.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 25dae9f0b205..a63057a03772 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -830,7 +830,7 @@ static void fcoe_fdmi_info(struct fc_lport
> *lport, struct net_device *netdev)
> if (rc) {
> printk(KERN_INFO "fcoe: Failed to retrieve FDMI
> "
> "information from netdev.\n");
> - return;
> + goto free_fdmi;
> }
>
> snprintf(fc_host_serial_number(lport->host),
> @@ -868,6 +868,7 @@ static void fcoe_fdmi_info(struct fc_lport
> *lport, struct net_device *netdev)
>
> /* Enable FDMI lport states */
> lport->fdmi_enabled = 1;
> +free_fdmi:
> kfree(fdmi);
> } else {
> lport->fdmi_enabled = 0;

Normally I would like to see goto labels for error paths outside
conditionals and at the end of the function. In this case it would
seem to be cleaner to put an else { } clause in the if (rc) above
around the snprintf() calls.

-Ewan

2020-07-16 08:14:48

by Jing Xiangfeng

Subject: Re: [PATCH v2] scsi: fcoe: add missed kfree() in an error path

On 2020/7/14 1:53, Ewan D. Milne wrote:
> See below.
>
> On Thu, 2020-07-09 at 20:05 +0800, Jing Xiangfeng wrote:
>> fcoe_fdmi_info() misses to call kfree() in an error path.
>> Add a label 'free_fdmi' and jump to it.
>>
>> Fixes: f07d46bbc9ba ("fcoe: Fix smatch warning in fcoe_fdmi_info
>> function")
>> Signed-off-by: Jing Xiangfeng <[email protected]>
>> ---
>> drivers/scsi/fcoe/fcoe.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
>> index 25dae9f0b205..a63057a03772 100644
>> --- a/drivers/scsi/fcoe/fcoe.c
>> +++ b/drivers/scsi/fcoe/fcoe.c
>> @@ -830,7 +830,7 @@ static void fcoe_fdmi_info(struct fc_lport
>> *lport, struct net_device *netdev)
>> if (rc) {
>> printk(KERN_INFO "fcoe: Failed to retrieve FDMI
>> "
>> "information from netdev.\n");
>> - return;
>> + goto free_fdmi;
>> }
>>
>> snprintf(fc_host_serial_number(lport->host),
>> @@ -868,6 +868,7 @@ static void fcoe_fdmi_info(struct fc_lport
>> *lport, struct net_device *netdev)
>>
>> /* Enable FDMI lport states */
>> lport->fdmi_enabled = 1;
>> +free_fdmi:
>> kfree(fdmi);
>> } else {
>> lport->fdmi_enabled = 0;
>
> Normally I would like to see goto labels for error paths outside
> conditionals and at the end of the function.

I agree.

In this case it would
> seem to be cleaner to put an else { } clause in the if (rc) above
> around the snprintf() calls.

It is ok with me. v1 is also a simpler way.

+Hannes, Which is preferable?

Thanks

>
> -Ewan
>
> .
>