diff mbox series

[v2] ethdev: avoid usage of uninit device info in bad port case

Message ID 1563883881-16258-1-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] ethdev: avoid usage of uninit device info in bad port case |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-Compile-Testing success Compile Testing PASS
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Andrew Rybchenko July 23, 2019, 12:11 p.m. UTC 
  rte_eth_dev_info_get() returns void and caller does know if the function
does its job or not. Changing of the return value to int would be
API/ABI breakage which requires deprecation process and cannot be
backported to stable branches. For now, make sure that device info is
initialized even in the case of invalid port ID.

Fixes: a30268e9a2d0 ("ethdev: reset whole dev info structure before filling")
Cc: stable@dpdk.org

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Thomas Monjalon July 23, 2019, 1:14 p.m. UTC | #1 
23/07/2019 14:11, Andrew Rybchenko:
> rte_eth_dev_info_get() returns void and caller does know if the function
> does its job or not. Changing of the return value to int would be
> API/ABI breakage which requires deprecation process and cannot be
> backported to stable branches. For now, make sure that device info is
> initialized even in the case of invalid port ID.
> 
> Fixes: a30268e9a2d0 ("ethdev: reset whole dev info structure before filling")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> +	/*
> +	 * Init dev_info before port_id check since caller does not have
> +	 * return status and does not know if get is successful or not.
> +	 */
> +	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));

If someone was using a canary to detect failure, it will be resetted.
Why is it urgent to have this workaround?
Can we wait one more release for the definitive fix with error code?

> +
>  	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>  	dev = &rte_eth_devices[port_id];
>  
> -	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>  	dev_info->rx_desc_lim = lim;
>  	dev_info->tx_desc_lim = lim;
>  	dev_info->device = dev->device;
Andrew Rybchenko July 23, 2019, 1:34 p.m. UTC | #2 
On 7/23/19 4:14 PM, Thomas Monjalon wrote:
> 23/07/2019 14:11, Andrew Rybchenko:
>> rte_eth_dev_info_get() returns void and caller does know if the function
>> does its job or not. Changing of the return value to int would be
>> API/ABI breakage which requires deprecation process and cannot be
>> backported to stable branches. For now, make sure that device info is
>> initialized even in the case of invalid port ID.
>>
>> Fixes: a30268e9a2d0 ("ethdev: reset whole dev info structure before filling")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> +	/*
>> +	 * Init dev_info before port_id check since caller does not have
>> +	 * return status and does not know if get is successful or not.
>> +	 */
>> +	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
> If someone was using a canary to detect failure, it will be resetted.

I've not thought about such ways to check. I would expected check
for not NULL device or driver_name. It is not defined behaviour of
the function to not touch dev_info in the case of bad port ID.

> Why is it urgent to have this workaround?

Nothing really urgent, but I still think that it is a right fix to be
applied and backported to stable branches.
I really met calls with invalid port ID and it took some time
to understand where uninitialized data come from.

> Can we wait one more release for the definitive fix with error code?

No strong opinion, but definitive fix will not be backported.

>> +
>>   	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>>   	dev = &rte_eth_devices[port_id];
>>   
>> -	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>>   	dev_info->rx_desc_lim = lim;
>>   	dev_info->tx_desc_lim = lim;
>>   	dev_info->device = dev->device;
>>
Thomas Monjalon July 23, 2019, 1:39 p.m. UTC | #3 
23/07/2019 15:34, Andrew Rybchenko:
> On 7/23/19 4:14 PM, Thomas Monjalon wrote:
> > 23/07/2019 14:11, Andrew Rybchenko:
> >> rte_eth_dev_info_get() returns void and caller does know if the function
> >> does its job or not. Changing of the return value to int would be
> >> API/ABI breakage which requires deprecation process and cannot be
> >> backported to stable branches. For now, make sure that device info is
> >> initialized even in the case of invalid port ID.
> >>
> >> Fixes: a30268e9a2d0 ("ethdev: reset whole dev info structure before filling")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >> ---
> >> --- a/lib/librte_ethdev/rte_ethdev.c
> >> +++ b/lib/librte_ethdev/rte_ethdev.c
> >> +	/*
> >> +	 * Init dev_info before port_id check since caller does not have
> >> +	 * return status and does not know if get is successful or not.
> >> +	 */
> >> +	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
> > If someone was using a canary to detect failure, it will be resetted.
> 
> I've not thought about such ways to check. I would expected check
> for not NULL device or driver_name. It is not defined behaviour of
> the function to not touch dev_info in the case of bad port ID.
> 
> > Why is it urgent to have this workaround?
> 
> Nothing really urgent, but I still think that it is a right fix to be
> applied and backported to stable branches.
> I really met calls with invalid port ID and it took some time
> to understand where uninitialized data come from.
> 
> > Can we wait one more release for the definitive fix with error code?
> 
> No strong opinion, but definitive fix will not be backported.

You're right.
Acked-by: Thomas Monjalon <thomas@monjalon.net>
Thomas Monjalon July 23, 2019, 6:45 p.m. UTC | #4 
23/07/2019 15:39, Thomas Monjalon:
> 23/07/2019 15:34, Andrew Rybchenko:
> > On 7/23/19 4:14 PM, Thomas Monjalon wrote:
> > > 23/07/2019 14:11, Andrew Rybchenko:
> > >> rte_eth_dev_info_get() returns void and caller does know if the function
> > >> does its job or not. Changing of the return value to int would be
> > >> API/ABI breakage which requires deprecation process and cannot be
> > >> backported to stable branches. For now, make sure that device info is
> > >> initialized even in the case of invalid port ID.
> > >>
> > >> Fixes: a30268e9a2d0 ("ethdev: reset whole dev info structure before filling")
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Applied, thanks
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 64191737c..17d183e1f 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -2552,10 +2552,15 @@  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 		.nb_mtu_seg_max = UINT16_MAX,
 	};
 
+	/*
+	 * Init dev_info before port_id check since caller does not have
+	 * return status and does not know if get is successful or not.
+	 */
+	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
+
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
 	dev = &rte_eth_devices[port_id];
 
-	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
 	dev_info->rx_desc_lim = lim;
 	dev_info->tx_desc_lim = lim;
 	dev_info->device = dev->device;