> -----Original Message-----
> From: Xu, Ting <ting.xu@intel.com>
> Sent: Monday, May 11, 2020 6:25 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; stable@dpdk.org
> Subject: [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang
>
> When set DCB in testpmd by clang, there is a segmentation fault.
> It is because the local variable rss_conf in get_eth_dcb_conf() is not cleared,
> so that the pointer member variable rss_key has a random address, which
> leads to an error in the following processing. This patch initialized the local
> variable rss_conf to avoid random address.
>
> Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ting Xu <ting.xu@intel.com>
> ---
> app/test-pmd/testpmd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 99bacddbf..1276476ca 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3408,6 +3408,7 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf
> *eth_conf,
> int32_t rc;
> struct rte_eth_rss_conf rss_conf;
>
> + memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf));
> /*
> * Builds up the correct configuration for dcb+vt based on the vlan
> tags array
> * given above, and the number of traffic classes available for use.
> --
> 2.17.1
Acked-by: Beilei Xing <beilei.xing@intel.com>
Tested-by: Huang, ZhiminX <zhiminx.huang@intel.com> Regards, HuangZhiMin -----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ting Xu Sent: Monday, May 11, 2020 6:25 PM To: dev@dpdk.org Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org Subject: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang When set DCB in testpmd by clang, there is a segmentation fault. It is because the local variable rss_conf in get_eth_dcb_conf() is not cleared, so that the pointer member variable rss_key has a random address, which leads to an error in the following processing. This patch initialized the local variable rss_conf to avoid random address. Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag") Cc: stable@dpdk.org Signed-off-by: Ting Xu <ting.xu@intel.com> --- app/test-pmd/testpmd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 99bacddbf..1276476ca 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -3408,6 +3408,7 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf, int32_t rc; struct rte_eth_rss_conf rss_conf; + memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf)); /* * Builds up the correct configuration for dcb+vt based on the vlan tags array * given above, and the number of traffic classes available for use. -- 2.17.1
When set DCB in testpmd by clang, there is a segmentation fault. It is because the local variable rss_conf in get_eth_dcb_conf() is not cleared, so that the pointer member variable rss_key has a random address, which leads to an error in the following processing. This patch initialized the local variable rss_conf to avoid random address. Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag") Cc: stable@dpdk.org Signed-off-by: Ting Xu <ting.xu@intel.com> --- app/test-pmd/testpmd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 99bacddbf..1276476ca 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -3408,6 +3408,7 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf, int32_t rc; struct rte_eth_rss_conf rss_conf; + memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf)); /* * Builds up the correct configuration for dcb+vt based on the vlan tags array * given above, and the number of traffic classes available for use. -- 2.17.1
On 5/11/2020 11:25 AM, Ting Xu wrote: > When set DCB in testpmd by clang, there is a segmentation fault. > It is because the local variable rss_conf in get_eth_dcb_conf() > is not cleared, so that the pointer member variable rss_key has > a random address, which leads to an error in the following > processing. This patch initialized the local variable rss_conf > to avoid random address. This is nothing really FreeBSD or clang issue, although it may be reproduced that environment, this is a pointer with random value issue. We may drop FreeBSD and clang reference to not create confusion. > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag") This commit looks unrelated, if not can you please explain why above commit causing the issue? > Cc: stable@dpdk.org > > Signed-off-by: Ting Xu <ting.xu@intel.com> > --- > app/test-pmd/testpmd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c > index 99bacddbf..1276476ca 100644 > --- a/app/test-pmd/testpmd.c > +++ b/app/test-pmd/testpmd.c > @@ -3408,6 +3408,7 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf, > int32_t rc; > struct rte_eth_rss_conf rss_conf; > > + memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf)); The variable is used in the 'else' leg, memset can be moved there, but more importantly should this be done in the 'rte_eth_dev_rss_hash_conf_get()' API. @Andrew, @Thomas, What do you think 'rte_eth_dev_rss_hash_conf_get()' memset the 'rss_conf' param before passing it to the PMD? To prevent issues like above in user application.
> -----Original Message----- > From: Yigit, Ferruh <ferruh.yigit@intel.com> > Sent: Tuesday, May 12, 2020 12:29 AM > To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei > <beilei.xing@intel.com>; Iremonger, Bernard > <bernard.iremonger@intel.com>; stable@dpdk.org; Andrew Rybchenko > <arybchenko@solarflare.com>; Thomas Monjalon <thomas@monjalon.net> > Subject: Re: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in > FreeBSD by clang > > On 5/11/2020 11:25 AM, Ting Xu wrote: > > When set DCB in testpmd by clang, there is a segmentation fault. > > It is because the local variable rss_conf in get_eth_dcb_conf() is not > > cleared, so that the pointer member variable rss_key has a random > > address, which leads to an error in the following processing. This > > patch initialized the local variable rss_conf to avoid random address. > > This is nothing really FreeBSD or clang issue, although it may be reproduced > that environment, this is a pointer with random value issue. We may drop > FreeBSD and clang reference to not create confusion. > OK, I will modify the commit log. > > > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag") > > This commit looks unrelated, if not can you please explain why above > commit causing the issue? > This is the bad commit the validation team find for this issue. Honestly speaking, I did not find the relation between this commit and the issue either. > > Cc: stable@dpdk.org > > > > Signed-off-by: Ting Xu <ting.xu@intel.com> > > --- > > app/test-pmd/testpmd.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index > > 99bacddbf..1276476ca 100644 > > --- a/app/test-pmd/testpmd.c > > +++ b/app/test-pmd/testpmd.c > > @@ -3408,6 +3408,7 @@ get_eth_dcb_conf(portid_t pid, struct > rte_eth_conf *eth_conf, > > int32_t rc; > > struct rte_eth_rss_conf rss_conf; > > > > + memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf)); > > The variable is used in the 'else' leg, memset can be moved there, but more > importantly should this be done in the 'rte_eth_dev_rss_hash_conf_get()' API. > > @Andrew, @Thomas, > > What do you think 'rte_eth_dev_rss_hash_conf_get()' memset the 'rss_conf' > param before passing it to the PMD? To prevent issues like above in user > application. I will move it to the else leg first in the v2 patch. Wait for more comments. Thanks!
When set DCB in testpmd, there is a segmentation fault. It is because the local variable rss_conf in get_eth_dcb_conf() is not cleared, so that the pointer member variable rss_key has a random address, which leads to an error in the following processing. This patch initialized the local variable rss_conf to avoid this situation. Fixes: ac7c491c3fec ("app/testpmd: fix DCB config") Cc: stable@dpdk.org Signed-off-by: Ting Xu <ting.xu@intel.com> --- v1->v2: modify commit log, move memset to else leg --- app/test-pmd/testpmd.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index a2d0be56b..9ad34421c 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -3448,6 +3448,8 @@ get_eth_dcb_conf(portid_t pid, struct rte_eth_conf *eth_conf, struct rte_eth_dcb_tx_conf *tx_conf = ð_conf->tx_adv_conf.dcb_tx_conf; + memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf)); + rc = rte_eth_dev_rss_hash_conf_get(pid, &rss_conf); if (rc != 0) return rc; -- 2.17.1
Hi Ting, > -----Original Message----- > From: Xu, Ting <ting.xu@intel.com> > Sent: Tuesday, May 12, 2020 3:17 AM > To: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei > <beilei.xing@intel.com>; Iremonger, Bernard > <bernard.iremonger@intel.com>; stable@dpdk.org; Andrew Rybchenko > <arybchenko@solarflare.com>; Thomas Monjalon <thomas@monjalon.net> > Subject: RE: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in > FreeBSD by clang > > > > > -----Original Message----- > > From: Yigit, Ferruh <ferruh.yigit@intel.com> > > Sent: Tuesday, May 12, 2020 12:29 AM > > To: Xu, Ting <ting.xu@intel.com>; dev@dpdk.org > > Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei > > <beilei.xing@intel.com>; Iremonger, Bernard > > <bernard.iremonger@intel.com>; stable@dpdk.org; Andrew Rybchenko > > <arybchenko@solarflare.com>; Thomas Monjalon > <thomas@monjalon.net> > > Subject: Re: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in > > FreeBSD by clang > > > > On 5/11/2020 11:25 AM, Ting Xu wrote: > > > When set DCB in testpmd by clang, there is a segmentation fault. > > > It is because the local variable rss_conf in get_eth_dcb_conf() is > > > not cleared, so that the pointer member variable rss_key has a > > > random address, which leads to an error in the following processing. > > > This patch initialized the local variable rss_conf to avoid random address. > > > > This is nothing really FreeBSD or clang issue, although it may be > > reproduced that environment, this is a pointer with random value > > issue. We may drop FreeBSD and clang reference to not create confusion. > > > > OK, I will modify the commit log. > > > > > > > Fixes: b57b66a97ebf ("app/testpmd: support mbuf dynamic flag") > > > > This commit looks unrelated, if not can you please explain why above > > commit causing the issue? > > > > This is the bad commit the validation team find for this issue. Honestly > speaking, I did not find the relation between this commit and the issue > either. > > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Ting Xu <ting.xu@intel.com> > > > --- > > > app/test-pmd/testpmd.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index > > > 99bacddbf..1276476ca 100644 > > > --- a/app/test-pmd/testpmd.c > > > +++ b/app/test-pmd/testpmd.c > > > @@ -3408,6 +3408,7 @@ get_eth_dcb_conf(portid_t pid, struct > > rte_eth_conf *eth_conf, > > > int32_t rc; > > > struct rte_eth_rss_conf rss_conf; > > > > > > +memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf)); > > > > The variable is used in the 'else' leg, memset can be moved there, but > > more importantly should this be done in the > 'rte_eth_dev_rss_hash_conf_get()' API. > > > > @Andrew, @Thomas, > > > > What do you think 'rte_eth_dev_rss_hash_conf_get()' memset the > 'rss_conf' > > param before passing it to the PMD? To prevent issues like above in > > user application. Seems like a good idea to me, to fix it in rte_eth_dev_rss_hash_conf_get() instead of in testpmd. > > I will move it to the else leg first in the v2 patch. Wait for more comments. > Thanks! Regards, Bernard.
Hi Ting, > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Ting Xu > Sent: Wednesday, May 13, 2020 11:08 AM > To: dev@dpdk.org > Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>; > arybchenko@solarflare.com; stable@dpdk.org > Subject: [dpdk-dev] [PATCH v4] app/testpmd: fix DCB set failure The fix is no longer in testpmd, so the commit line should be "ethdev: fix DCB set failure". > When set DCB in testpmd, there is a segmentation fault. It is because the > local variable rss_conf in get_eth_dcb_conf() is not cleared, so that the > pointer member variable rss_key has a random address, which leads to an > error in the following processing. This patch initialized the local variable > rss_conf to avoid this situation. The commit message should probably be revised. > > Fixes: ac7c491c3fec ("app/testpmd: fix DCB config") Also the Fixes line may need to change. > Cc: stable@dpdk.org > > Signed-off-by: Ting Xu <ting.xu@intel.com> > > --- > 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 | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 8e10a6fc3..1f6ca742a 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -3549,6 +3549,8 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id, > { > struct rte_eth_dev *dev; > > + memset(rss_conf, 0, sizeof(*rss_conf)); The memset call should be moved to after the error checks to just before the function is called. > + > 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); > -- > 2.17.1 Regards, Bernard.
When set DCB in testpmd, there is a segmentation fault. It is because the local variable rss_conf in get_eth_dcb_conf() is not cleared, so that the pointer member variable rss_key has a random address, which leads to an error in the following processing. This patch initialized the local variable rss_conf to avoid this situation. Fixes: ac7c491c3fec ("app/testpmd: fix DCB config") Cc: stable@dpdk.org Signed-off-by: Ting Xu <ting.xu@intel.com> --- 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 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 8e10a6fc3..6c86d7a73 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -3549,6 +3549,8 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id, { struct rte_eth_dev *dev; + memset(&rss_conf, 0, sizeof(struct rte_eth_rss_conf)); + 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); -- 2.17.1
When set DCB in testpmd, there is a segmentation fault. It is because the local variable rss_conf in get_eth_dcb_conf() is not cleared, so that the pointer member variable rss_key has a random address, which leads to an error in the following processing. This patch initialized the local variable rss_conf to avoid this situation. Fixes: ac7c491c3fec ("app/testpmd: fix DCB config") Cc: stable@dpdk.org Signed-off-by: Ting Xu <ting.xu@intel.com> --- 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 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 8e10a6fc3..1f6ca742a 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -3549,6 +3549,8 @@ rte_eth_dev_rss_hash_conf_get(uint16_t port_id, { struct rte_eth_dev *dev; + memset(rss_conf, 0, sizeof(*rss_conf)); + 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); -- 2.17.1
13/05/2020 10:54, Iremonger, Bernard:
> The fix is no longer in testpmd, so the commit line should be "ethdev: fix DCB set failure".
In general, the word "failure" (or error, etc) is not needed.
Because it is a fix, we know there was a failure before :)
And specifically here, you are not fixing just DCB.
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>
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
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.
On 5/12/2020 11:13 AM, Ting Xu wrote:
> When set DCB in testpmd, there is a segmentation fault. It is
> because the local variable rss_conf in get_eth_dcb_conf()
> is not cleared, so that the pointer member variable rss_key has
> a random address, which leads to an error in the following
> processing. This patch initialized the local variable rss_conf
> to avoid this situation.
>
> Fixes: ac7c491c3fec ("app/testpmd: fix DCB config")
> 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.