[v5] ethdev: fix DCB set failure
Checks
Commit Message
When rte_eth_dev_rss_hash_conf_get() is called, if the variable
rss_conf is not initialized, the pointer member variable rss_key
may have a random address, which leads to an error in the following
processing. This patch initialized the variable rss_conf to avoid
this situation.
Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and key")
Cc: stable@dpdk.org
Signed-off-by: Ting Xu <ting.xu@intel.com>
---
v4->v5: modify commit log
V3->v4: fix build failure
v2->v3: move memset to rte_eth_dev_rss_hash_conf_get from testpmd
v1->v2: modify commit log, move memset to else leg
---
lib/librte_ethdev/rte_ethdev.c | 1 +
1 file changed, 1 insertion(+)
Comments
On 5/13/2020 6:16 PM, Ting Xu wrote:
> When rte_eth_dev_rss_hash_conf_get() is called, if the variable
> rss_conf is not initialized, the pointer member variable rss_key
> may have a random address, which leads to an error in the following
> processing. This patch initialized the variable rss_conf to avoid
> this situation.
>
> Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and key")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ting Xu <ting.xu@intel.com>
>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Applied to dpdk-next-net/master, thanks.
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ting Xu
> Sent: Wednesday, May 13, 2020 6:16 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v5] ethdev: fix DCB set failure
>
> When rte_eth_dev_rss_hash_conf_get() is called, if the variable rss_conf is
> not initialized, the pointer member variable rss_key may have a random
> address, which leads to an error in the following processing. This patch
> initialized the variable rss_conf to avoid this situation.
>
> Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and key")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ting Xu <ting.xu@intel.com>
>
> ---
> v4->v5: modify commit log
> V3->v4: fix build failure
> v2->v3: move memset to rte_eth_dev_rss_hash_conf_get from testpmd
> v1->v2: modify commit log, move memset to else leg
> ---
> lib/librte_ethdev/rte_ethdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 8e10a6fc3..b20c6bb2d 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3552,6 +3552,7 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> dev = &rte_eth_devices[port_id];
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get,
> -ENOTSUP);
> + memset(rss_conf, 0, sizeof(*rss_conf));
> return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
> rss_conf));
> }
> --
> 2.17.1
The commit line should probably be reworded to describe the revised fix.
Otherwise
Reviewed-by: Bernard Iremonger <bernard.iremonger@intel.com>
CC net/nfp maintainer
On 5/13/20 8:16 PM, Ting Xu wrote:
> When rte_eth_dev_rss_hash_conf_get() is called, if the variable
> rss_conf is not initialized, the pointer member variable rss_key
> may have a random address, which leads to an error in the following
> processing. This patch initialized the variable rss_conf to avoid
> this situation.
>
> Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and key")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ting Xu <ting.xu@intel.com>
>
> ---
> v4->v5: modify commit log
> V3->v4: fix build failure
> v2->v3: move memset to rte_eth_dev_rss_hash_conf_get from testpmd
> v1->v2: modify commit log, move memset to else leg
> ---
> lib/librte_ethdev/rte_ethdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 8e10a6fc3..b20c6bb2d 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3552,6 +3552,7 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> dev = &rte_eth_devices[port_id];
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
> + memset(rss_conf, 0, sizeof(*rss_conf));
> return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
> rss_conf));
> }
>
The patch breaks getting of RSS key since it resets rss_conf->rss_key to
NULL, but all drivers treat it as
a pointer to buffer to copy RSS key to.
Almost all driver just skip RSS key copy if the pointer
is NULL.
I think it should be removed from next-net or improved
to reset rss_hf only since rss_key_len may be treated
as a size of rss_key buffer. The API is not clear at
the place.
It looks like net/nfp is the only driver which does
not check that that pointer is not NULL before coping
key to it.
Hi
From: Andrew Rybchenko
> CC net/nfp maintainer
>
> On 5/13/20 8:16 PM, Ting Xu wrote:
> > When rte_eth_dev_rss_hash_conf_get() is called, if the variable
> > rss_conf is not initialized, the pointer member variable rss_key may
> > have a random address, which leads to an error in the following
> > processing. This patch initialized the variable rss_conf to avoid this
> > situation.
> >
> > Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and
> > key")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ting Xu <ting.xu@intel.com>
> >
> > ---
> > v4->v5: modify commit log
> > V3->v4: fix build failure
> > v2->v3: move memset to rte_eth_dev_rss_hash_conf_get from testpmd
> > v1->v2: modify commit log, move memset to else leg
> > ---
> > lib/librte_ethdev/rte_ethdev.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c
> > b/lib/librte_ethdev/rte_ethdev.c index 8e10a6fc3..b20c6bb2d 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -3552,6 +3552,7 @@ rte_eth_dev_rss_hash_conf_get(uint16_t
> port_id,
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > dev = &rte_eth_devices[port_id];
> > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get,
> -ENOTSUP);
> > + memset(rss_conf, 0, sizeof(*rss_conf));
> > return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
> > rss_conf));
> > }
> >
>
> The patch breaks getting of RSS key since it resets rss_conf->rss_key to
> NULL, but all drivers treat it as a pointer to buffer to copy RSS key to.
> Almost all driver just skip RSS key copy if the pointer is NULL.
>
> I think it should be removed from next-net or improved to reset rss_hf only
> since rss_key_len may be treated as a size of rss_key buffer. The API is not
> clear at the place.
>
> It looks like net/nfp is the only driver which does not check that that pointer
> is not NULL before coping key to it.
+1
Looks like this breaks also Testpmd "show port 0 rss-hash key" command.
Matan
On 5/18/2020 2:32 PM, Andrew Rybchenko wrote:
> CC net/nfp maintainer
>
> On 5/13/20 8:16 PM, Ting Xu wrote:
>> When rte_eth_dev_rss_hash_conf_get() is called, if the variable
>> rss_conf is not initialized, the pointer member variable rss_key
>> may have a random address, which leads to an error in the following
>> processing. This patch initialized the variable rss_conf to avoid
>> this situation.
>>
>> Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and key")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ting Xu <ting.xu@intel.com>
>>
>> ---
>> v4->v5: modify commit log
>> V3->v4: fix build failure
>> v2->v3: move memset to rte_eth_dev_rss_hash_conf_get from testpmd
>> v1->v2: modify commit log, move memset to else leg
>> ---
>> lib/librte_ethdev/rte_ethdev.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 8e10a6fc3..b20c6bb2d 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -3552,6 +3552,7 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
>> + memset(rss_conf, 0, sizeof(*rss_conf));
>> return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
>> rss_conf));
>> }
>>
>
> The patch breaks getting of RSS key since it resets rss_conf->rss_key to
> NULL, but all drivers treat it as
> a pointer to buffer to copy RSS key to.
:(
Of course it is a pointer to copy the RSS key, no idea what I was thinking.
Thanks for catching this before goes into main repo.
> Almost all driver just skip RSS key copy if the pointer
> is NULL.
>
> I think it should be removed from next-net or improved
+1, I will drop the ethdev patch, and replace it with the testpmd one.
> to reset rss_hf only since rss_key_len may be treated
> as a size of rss_key buffer. The API is not clear at
> the place.
>
> It looks like net/nfp is the only driver which does
> not check that that pointer is not NULL before coping
> key to it.
>
On 5/18/2020 3:57 PM, Matan Azrad wrote:
>
> Hi
>
> From: Andrew Rybchenko
>> CC net/nfp maintainer
>>
>> On 5/13/20 8:16 PM, Ting Xu wrote:
>>> When rte_eth_dev_rss_hash_conf_get() is called, if the variable
>>> rss_conf is not initialized, the pointer member variable rss_key may
>>> have a random address, which leads to an error in the following
>>> processing. This patch initialized the variable rss_conf to avoid this
>>> situation.
>>>
>>> Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and
>>> key")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ting Xu <ting.xu@intel.com>
>>>
>>> ---
>>> v4->v5: modify commit log
>>> V3->v4: fix build failure
>>> v2->v3: move memset to rte_eth_dev_rss_hash_conf_get from testpmd
>>> v1->v2: modify commit log, move memset to else leg
>>> ---
>>> lib/librte_ethdev/rte_ethdev.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>> b/lib/librte_ethdev/rte_ethdev.c index 8e10a6fc3..b20c6bb2d 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -3552,6 +3552,7 @@ rte_eth_dev_rss_hash_conf_get(uint16_t
>> port_id,
>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> dev = &rte_eth_devices[port_id];
>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get,
>> -ENOTSUP);
>>> + memset(rss_conf, 0, sizeof(*rss_conf));
>>> return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
>>> rss_conf));
>>> }
>>>
>>
>> The patch breaks getting of RSS key since it resets rss_conf->rss_key to
>> NULL, but all drivers treat it as a pointer to buffer to copy RSS key to.
>> Almost all driver just skip RSS key copy if the pointer is NULL.
>>
>> I think it should be removed from next-net or improved to reset rss_hf only
>> since rss_key_len may be treated as a size of rss_key buffer. The API is not
>> clear at the place.
>>
>> It looks like net/nfp is the only driver which does not check that that pointer
>> is not NULL before coping key to it.
>
>
> +1
>
> Looks like this breaks also Testpmd "show port 0 rss-hash key" command.
>
Hi Matan, it is clearly wrong, I will drop it.
On 5/13/2020 2:28 PM, Ferruh Yigit wrote:
> On 5/13/2020 6:16 PM, Ting Xu wrote:
>> When rte_eth_dev_rss_hash_conf_get() is called, if the variable
>> rss_conf is not initialized, the pointer member variable rss_key
>> may have a random address, which leads to an error in the following
>> processing. This patch initialized the variable rss_conf to avoid
>> this situation.
>>
>> Fixes: 16321de09396 ("ethdev: allow to get RSS hash functions and key")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ting Xu <ting.xu@intel.com>
>>
>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
> Applied to dpdk-next-net/master, thanks.
>
Dropped from next-net.
@@ -3552,6 +3552,7 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id,
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
dev = &rte_eth_devices[port_id];
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rss_hash_conf_get, -ENOTSUP);
+ memset(rss_conf, 0, sizeof(*rss_conf));
return eth_err(port_id, (*dev->dev_ops->rss_hash_conf_get)(dev,
rss_conf));
}