[dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang

[dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang

[Ting Xu]

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

Re: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang

[Xing, Beilei]

> -----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>

Re: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang

[Huang, ZhiminX]

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

Re: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang

[Ferruh Yigit]

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.

Re: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang

[Xu, Ting]

> -----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!

Re: [dpdk-dev] [PATCH v1] app/testpmd: fix DCB set failure in FreeBSD by clang

[Iremonger, Bernard]

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.

[dpdk-dev] [PATCH v2] app/testpmd: fix DCB set failure

[Ting Xu]

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 =
 				&eth_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

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix DCB set failure

[Ferruh Yigit]

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.

[dpdk-dev] [PATCH v3] app/testpmd: fix DCB set failure

[Ting Xu]

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

[dpdk-dev] [PATCH v4] app/testpmd: fix DCB set failure

[Ting Xu]

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

Re: [dpdk-dev] [PATCH v4] app/testpmd: fix DCB set failure

[Iremonger, 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.

Re: [dpdk-dev] [PATCH v4] app/testpmd: fix DCB set failure

[Thomas Monjalon]

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.

[dpdk-dev] [PATCH v5] ethdev: fix DCB set failure

[Ting Xu]

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

Re: [dpdk-dev] [PATCH v5] ethdev: fix DCB set failure

[Ferruh Yigit]

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.

Re: [dpdk-dev] [dpdk-stable] [PATCH v5] ethdev: fix DCB set failure

[Ferruh Yigit]

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.

Re: [dpdk-dev] [PATCH v5] ethdev: fix DCB set failure

[Iremonger, Bernard]

> -----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>

Re: [dpdk-dev] [PATCH v5] ethdev: fix DCB set failure

[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.

Re: [dpdk-dev] [PATCH v5] ethdev: fix DCB set failure

[Matan Azrad]

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

Re: [dpdk-dev] [PATCH v5] ethdev: fix DCB set failure

[Ferruh Yigit]

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.

Re: [dpdk-dev] [PATCH v5] ethdev: fix DCB set failure

[Ferruh Yigit]

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.
>