diff mbox series

[2/3] net/kni: fix rewritten return value

Message ID 1619063789-60922-3-git-send-email-humin29@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series bugfix for kni |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

humin (Q) April 22, 2021, 3:56 a.m. UTC 
  Return value of function 'eth_kni_dev_stop' passed to 'ret' is
rewritten later, and this is unreasonable.

This patch fixes it.

Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/kni/rte_eth_kni.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Ferruh Yigit April 26, 2021, 1:18 p.m. UTC | #1 
On 4/22/2021 4:56 AM, Min Hu (Connor) wrote:
> Return value of function 'eth_kni_dev_stop' passed to 'ret' is
> rewritten later, and this is unreasonable.
> 
> This patch fixes it.
> 
> Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/kni/rte_eth_kni.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
> index 9ce74e5..067584c 100644
> --- a/drivers/net/kni/rte_eth_kni.c
> +++ b/drivers/net/kni/rte_eth_kni.c
> @@ -211,6 +211,11 @@ eth_kni_close(struct rte_eth_dev *eth_dev)
>  		return 0;
>  
>  	ret = eth_kni_dev_stop(eth_dev);
> +	if (ret) {
> +		PMD_LOG(WARNING, "Not able to stop kni for %s",
> +			eth_dev->data->name);
> +		return ret;
> +	}
>  

'eth_kni_close()' is called by 'eth_kni_remove()', and returning here without
setting 'eth_dev->data->mac_addrs' to NULL, 'eth_kni_remove()' may crash.

And the close ops is to not use the device anymore, there is not much to do if
stop() fails here. So what do you think to log but not return on the
'eth_kni_dev_stop()' failure?


>  	/* mac_addrs must not be freed alone because part of dev_private */
>  	eth_dev->data->mac_addrs = NULL;
>
humin (Q) April 27, 2021, 2:09 a.m. UTC | #2 
在 2021/4/26 21:18, Ferruh Yigit 写道:
> On 4/22/2021 4:56 AM, Min Hu (Connor) wrote:
>> Return value of function 'eth_kni_dev_stop' passed to 'ret' is
>> rewritten later, and this is unreasonable.
>>
>> This patch fixes it.
>>
>> Fixes: 62024eb82756 ("ethdev: change stop operation callback to return int")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/net/kni/rte_eth_kni.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
>> index 9ce74e5..067584c 100644
>> --- a/drivers/net/kni/rte_eth_kni.c
>> +++ b/drivers/net/kni/rte_eth_kni.c
>> @@ -211,6 +211,11 @@ eth_kni_close(struct rte_eth_dev *eth_dev)
>>   		return 0;
>>   
>>   	ret = eth_kni_dev_stop(eth_dev);
>> +	if (ret) {
>> +		PMD_LOG(WARNING, "Not able to stop kni for %s",
>> +			eth_dev->data->name);
>> +		return ret;
>> +	}
>>   
> 
> 'eth_kni_close()' is called by 'eth_kni_remove()', and returning here without
> setting 'eth_dev->data->mac_addrs' to NULL, 'eth_kni_remove()' may crash.
> 
> And the close ops is to not use the device anymore, there is not much to do if
> stop() fails here. So what do you think to log but not return on the
> 'eth_kni_dev_stop()' failure?
> 
Agreed, fixed in v2, thanks.
> 
>>   	/* mac_addrs must not be freed alone because part of dev_private */
>>   	eth_dev->data->mac_addrs = NULL;
>>
> 
> .
>
diff mbox series

Patch

diff --git a/drivers/net/kni/rte_eth_kni.c b/drivers/net/kni/rte_eth_kni.c
index 9ce74e5..067584c 100644
--- a/drivers/net/kni/rte_eth_kni.c
+++ b/drivers/net/kni/rte_eth_kni.c
@@ -211,6 +211,11 @@  eth_kni_close(struct rte_eth_dev *eth_dev)
 		return 0;
 
 	ret = eth_kni_dev_stop(eth_dev);
+	if (ret) {
+		PMD_LOG(WARNING, "Not able to stop kni for %s",
+			eth_dev->data->name);
+		return ret;
+	}
 
 	/* mac_addrs must not be freed alone because part of dev_private */
 	eth_dev->data->mac_addrs = NULL;