DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.
@ 2020-03-04 14:05 Tencent TGW team
  2020-03-04 14:16 ` Andrew Rybchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Tencent TGW team @ 2020-03-04 14:05 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko; +Cc: dev, Tencent TGW team

When compiling with -O0,
the compiler does not optimize two memory accesses into one.
Leads to accessing a null pointer when calling the RX callback.
The way to access the TX callback is correct.

Signed-off-by: Tencent TGW team <tgw_team@tencent.com>
---
 lib/librte_ethdev/rte_ethdev.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad1..35eb580ff 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 				     rx_pkts, nb_pkts);
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
-		struct rte_eth_rxtx_callback *cb =
-				dev->post_rx_burst_cbs[queue_id];
-
+	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
+	if (unlikely(cb != NULL)) {
 		do {
 			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
 						nb_pkts, cb->param);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.
  2020-03-04 14:05 [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback Tencent TGW team
@ 2020-03-04 14:16 ` Andrew Rybchenko
       [not found]   ` <61a6b7d5533643d692c40dc0ab1a2cdc@tencent.com>
  2020-03-04 16:15 ` Stephen Hemminger
  2020-03-04 17:33 ` [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback ZY Qiu
  2 siblings, 1 reply; 21+ messages in thread
From: Andrew Rybchenko @ 2020-03-04 14:16 UTC (permalink / raw)
  To: Tencent TGW team, Thomas Monjalon, Ferruh Yigit; +Cc: dev, Tencent TGW team

On 3/4/20 5:05 PM, Tencent TGW team wrote:
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when calling the RX callback.
> The way to access the TX callback is correct.

It looks like the patch is not passed through check-git-log.sh.
RX -> Rx, TX -> Tx

> 
> Signed-off-by: Tencent TGW team <tgw_team@tencent.com>

If I'm not mistaken, it must be a person here, not team.

> ---
>  lib/librte_ethdev/rte_ethdev.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..35eb580ff 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>  				     rx_pkts, nb_pkts);
>  
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -		struct rte_eth_rxtx_callback *cb =
> -				dev->post_rx_burst_cbs[queue_id];
> -
> +	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> +	if (unlikely(cb != NULL)) {
>  		do {
>  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>  						nb_pkts, cb->param);
> 

Sorry, but I don't understand. I don't see the difference in
potential NULL pointer deference above. What is the compiler? Version?

Or is it a race condition with queue post Rx burst callback
removal  while traffic is running?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.
  2020-03-04 14:05 [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback Tencent TGW team
  2020-03-04 14:16 ` Andrew Rybchenko
@ 2020-03-04 16:15 ` Stephen Hemminger
  2020-03-04 16:38   ` [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail) tgw_team
  2020-03-04 17:33 ` [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback ZY Qiu
  2 siblings, 1 reply; 21+ messages in thread
From: Stephen Hemminger @ 2020-03-04 16:15 UTC (permalink / raw)
  To: Tencent TGW team
  Cc: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev, Tencent TGW team

On Wed,  4 Mar 2020 22:05:43 +0800
Tencent TGW team <quzeyao@gmail.com> wrote:

> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when calling the RX callback.
> The way to access the TX callback is correct.
> 
> Signed-off-by: Tencent TGW team <tgw_team@tencent.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..35eb580ff 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>  				     rx_pkts, nb_pkts);
>  
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -		struct rte_eth_rxtx_callback *cb =
> -				dev->post_rx_burst_cbs[queue_id];
> -
> +	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> +	if (unlikely(cb != NULL)) {
>  		do {
>  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>  						nb_pkts, cb->param);

To fix TOCTOU like this you need to use something like READ_ONCE()
which does cast to volatile.

For Developer Certificate of Origin (Signed-off-by) you must use a person
not a team. It is a legal requirement. If you don't know what DCO is you should
read what it means.

Developer's Certificate of Origin 1.1
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

then you just add a line saying::

	Signed-off-by: Random J Developer <random@developer.example.org>

using your real name (sorry, no pseudonyms or anonymous contributions.)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.
       [not found]   ` <61a6b7d5533643d692c40dc0ab1a2cdc@tencent.com>
@ 2020-03-04 16:26     ` tgw_team
  0 siblings, 0 replies; 21+ messages in thread
From: tgw_team(腾讯网关团队) @ 2020-03-04 16:26 UTC (permalink / raw)
  To: Andrew Rybchenko, Tencent TGW team, Thomas Monjalon, Ferruh Yigit; +Cc: dev

As a newcomer, I'm sorry for my mistakes. I will try to do better in the future.

>On 3/4/20 5:05 PM, Tencent TGW team wrote:
>> When compiling with -O0,
>> the compiler does not optimize two memory accesses into one.
>> Leads to accessing a null pointer when calling the RX callback.
>> The way to access the TX callback is correct.
>
>It looks like the patch is not passed through check-git-log.sh.
>RX -> Rx, TX -> Tx

Sorry, I will check and refine in patch v2.

>>
>> 
>> Signed-off-by: Tencent TGW team <tgw_team@tencent.com>

>If I'm not mistaken, it must be a person here, not team.

Is there any way to submit patches as a team?

>> ---
>>  lib/librte_ethdev/rte_ethdev.h | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index d1a593ad1..35eb580ff 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>>                                     rx_pkts, nb_pkts);
>>  
>>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>> -     if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
>> -             struct rte_eth_rxtx_callback *cb =
>> -                             dev->post_rx_burst_cbs[queue_id];
>> -
>> +     struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
>> +     if (unlikely(cb != NULL)) {
>>                do {
>>                        nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>>                                                nb_pkts, cb->param);
>> 

>Sorry, but I don't understand. I don't see the difference in
>potential NULL pointer deference above. What is the compiler? Version?

gcc version 7.4.0
I think this problem has nothing to do with the compiler version.

>Or is it a race condition with queue post Rx burst callback
>removal  while traffic is running?

You're right.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail)
  2020-03-04 16:15 ` Stephen Hemminger
@ 2020-03-04 16:38   ` tgw_team
  2020-03-04 17:37     ` Stephen Hemminger
  0 siblings, 1 reply; 21+ messages in thread
From: tgw_team(腾讯网关团队) @ 2020-03-04 16:38 UTC (permalink / raw)
  To: Stephen Hemminger, Tencent TGW team
  Cc: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev

Sorry, I`ll use a real name in patch v2.

I don't think this is a TOCTOU question.
The original code works fine when compiled with the -O3 option.
At this point the compiler will optimize to one memory access.
But when compiled with -O0, there will be two memory accesses, which is wrong.
This change was modified with reference to the rte_eth_tx_burst function.

On Wed,  4 Mar 2020 22:05:43 +0800
Tencent TGW team <quzeyao@gmail.com> wrote:

> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when calling the RX callback.
> The way to access the TX callback is correct.
> 
> Signed-off-by: Tencent TGW team <tgw_team@tencent.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..35eb580ff 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>                                     rx_pkts, nb_pkts);
>  
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -     if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -             struct rte_eth_rxtx_callback *cb =
> -                             dev->post_rx_burst_cbs[queue_id];
> -
> +     struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> +     if (unlikely(cb != NULL)) {
>                do {
>                        nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>                                                nb_pkts, cb->param);

To fix TOCTOU like this you need to use something like READ_ONCE()
which does cast to volatile.

For Developer Certificate of Origin (Signed-off-by) you must use a person
not a team. It is a legal requirement. If you don't know what DCO is you should
read what it means.

Developer's Certificate of Origin 1.1
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

then you just add a line saying::

        Signed-off-by: Random J Developer <random@developer.example.org>

using your real name (sorry, no pseudonyms or anonymous contributions.)

    

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-04 14:05 [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback Tencent TGW team
  2020-03-04 14:16 ` Andrew Rybchenko
  2020-03-04 16:15 ` Stephen Hemminger
@ 2020-03-04 17:33 ` ZY Qiu
  2020-03-04 17:56   ` Stephen Hemminger
                     ` (2 more replies)
  2 siblings, 3 replies; 21+ messages in thread
From: ZY Qiu @ 2020-03-04 17:33 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko; +Cc: dev, ZY Qiu

When compiling with -O0,
the compiler does not optimize two memory accesses into one.
Leads to accessing a null pointer when queue post Rx burst callback
removal while traffic is running.
See rte_eth_tx_burst function.

Signed-off-by: ZY Qiu <tgw_team@tencent.com>
---
 lib/librte_ethdev/rte_ethdev.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad1..35eb580ff 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 				     rx_pkts, nb_pkts);
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
-		struct rte_eth_rxtx_callback *cb =
-				dev->post_rx_burst_cbs[queue_id];
-
+	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
+	if (unlikely(cb != NULL)) {
 		do {
 			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
 						nb_pkts, cb->param);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail)
  2020-03-04 16:38   ` [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail) tgw_team
@ 2020-03-04 17:37     ` Stephen Hemminger
  2020-03-04 17:44       ` [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback tgw_team
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Hemminger @ 2020-03-04 17:37 UTC (permalink / raw)
  To: tgw_team(腾讯网关团队)
  Cc: Tencent TGW team, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev

On Wed, 4 Mar 2020 16:38:13 +0000
tgw_team(腾讯网关团队) <tgw_team@tencent.com> wrote:

> Sorry, I`ll use a real name in patch v2.
> 
> I don't think this is a TOCTOU question.
> The original code works fine when compiled with the -O3 option.
> At this point the compiler will optimize to one memory access.
> But when compiled with -O0, there will be two memory accesses, which is wrong.
> This change was modified with reference to the rte_eth_tx_burst function.

There is nothing C standard that says compiler has to do it either way.
The optimizer may decide to do two memory accesses or one.
Depending on that in anyway is bad practice.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.
  2020-03-04 17:37     ` Stephen Hemminger
@ 2020-03-04 17:44       ` tgw_team
  0 siblings, 0 replies; 21+ messages in thread
From: tgw_team(腾讯网关团队) @ 2020-03-04 17:44 UTC (permalink / raw)
  To: Stephen Hemminger, Tencent TGW team, Thomas Monjalon,
	Ferruh Yigit, Andrew Rybchenko, dev


So in my patch it was changed to only one memory access.

________________________________
发件人: Stephen Hemminger <stephen@networkplumber.org>
发送时间: 2020年3月5日 1:37
收件人: tgw_team(腾讯网关团队)
抄送: Tencent TGW team; Thomas Monjalon; Ferruh Yigit; Andrew Rybchenko; dev@dpdk.org
主题: Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail)

On Wed, 4 Mar 2020 16:38:13 +0000
tgw_team(腾讯网关团队) <tgw_team@tencent.com> wrote:

> Sorry, I`ll use a real name in patch v2.
>
> I don't think this is a TOCTOU question.
> The original code works fine when compiled with the -O3 option.
> At this point the compiler will optimize to one memory access.
> But when compiled with -O0, there will be two memory accesses, which is wrong.
> This change was modified with reference to the rte_eth_tx_burst function.

There is nothing C standard that says compiler has to do it either way.
The optimizer may decide to do two memory accesses or one.
Depending on that in anyway is bad practice.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-04 17:33 ` [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback ZY Qiu
@ 2020-03-04 17:56   ` Stephen Hemminger
  2020-03-04 18:31     ` tgw_team
  2020-03-05  9:19   ` Bruce Richardson
  2020-03-05 16:47   ` [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback ZY Qiu
  2 siblings, 1 reply; 21+ messages in thread
From: Stephen Hemminger @ 2020-03-04 17:56 UTC (permalink / raw)
  To: ZY Qiu; +Cc: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev, ZY Qiu

On Thu,  5 Mar 2020 01:33:49 +0800
ZY Qiu <quzeyao@gmail.com> wrote:

> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when queue post Rx burst callback
> removal while traffic is running.
> See rte_eth_tx_burst function.
> 
> Signed-off-by: ZY Qiu <tgw_team@tencent.com>

This is a problem many places in DPDK. You said it was related to -O0
but that is just what is causing a more generic problem to be exposed.
Your solution is not sufficient.

DPDK is sloppy in several places in handling memory ordering issues     
    https://en.wikipedia.org/wiki/Memory_ordering

It should have a macro to do RTE_READ_ONCE(). Several drives have this,
the Linux kernel has it, Liburcu has it.

The macro RTE_READ_ONCE() can then be used in many places in DPDK.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-04 17:56   ` Stephen Hemminger
@ 2020-03-04 18:31     ` tgw_team
  0 siblings, 0 replies; 21+ messages in thread
From: tgw_team(腾讯网关团队) @ 2020-03-04 18:31 UTC (permalink / raw)
  To: Stephen Hemminger, ZY Qiu, Thomas Monjalon, Ferruh Yigit,
	Andrew Rybchenko, dev

Thank you raising your concerns.

I mean, the original wrong code, but using -O3 optimization yielded a correct result.
My patch makes the effects of -O3 and -O0 consistent.

Unlike other signals that require busy wait, this callback pointer only needs to be read once. So I don't think memory barriers and volatile are needed here.
    
>On Thu,  5 Mar 2020 01:33:49 +0800
>ZY Qiu <quzeyao@gmail.com> wrote:
>
>> When compiling with -O0,
>> the compiler does not optimize two memory accesses into one.
>> Leads to accessing a null pointer when queue post Rx burst callback
>> removal while traffic is running.
>> See rte_eth_tx_burst function.
>> 
>> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
>
>This is a problem many places in DPDK. You said it was related to -O0
>but that is just what is causing a more generic problem to be exposed.
>Your solution is not sufficient.
>
>DPDK is sloppy in several places in handling memory ordering issues     
>    https://en.wikipedia.org/wiki/Memory_ordering
>
>It should have a macro to do RTE_READ_ONCE(). Several drives have this,
>the Linux kernel has it, Liburcu has it.
>
>The macro RTE_READ_ONCE() can then be used in many places in DPDK.


    

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-04 17:33 ` [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback ZY Qiu
  2020-03-04 17:56   ` Stephen Hemminger
@ 2020-03-05  9:19   ` Bruce Richardson
  2020-03-05 11:27     ` Ananyev, Konstantin
  2020-03-05 16:47   ` [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback ZY Qiu
  2 siblings, 1 reply; 21+ messages in thread
From: Bruce Richardson @ 2020-03-05  9:19 UTC (permalink / raw)
  To: ZY Qiu; +Cc: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dev, ZY Qiu

On Thu, Mar 05, 2020 at 01:33:49AM +0800, ZY Qiu wrote:
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when queue post Rx burst callback
> removal while traffic is running.
> See rte_eth_tx_burst function.
> 
> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..35eb580ff 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>  				     rx_pkts, nb_pkts);
>  
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -		struct rte_eth_rxtx_callback *cb =
> -				dev->post_rx_burst_cbs[queue_id];
> -
> +	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> +	if (unlikely(cb != NULL)) {
>  		do {
>  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>  						nb_pkts, cb->param);
> -- 
> 2.17.1
While I don't have an issue with this fix, can you explain as to why this is a
problem that needs to be fixed? Normally TOCTOU issues are flagged and
fixed for external resources e.g. files, that can be modified between check
and use, but this is just referencing internal data in the program itself,
so I'm wondering what the risk is? From a security viewpoint if an attacker
can modify the function pointers in our code, is it not already "game over"
for keeping the running program safe?

/Bruce

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-05  9:19   ` Bruce Richardson
@ 2020-03-05 11:27     ` Ananyev, Konstantin
  2020-03-05 14:23       ` tgw_team
  2020-03-05 14:47       ` Liang, Ma
  0 siblings, 2 replies; 21+ messages in thread
From: Ananyev, Konstantin @ 2020-03-05 11:27 UTC (permalink / raw)
  To: Richardson, Bruce, ZY Qiu
  Cc: Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko, dev, ZY Qiu



> 
> On Thu, Mar 05, 2020 at 01:33:49AM +0800, ZY Qiu wrote:
> > When compiling with -O0,
> > the compiler does not optimize two memory accesses into one.
> > Leads to accessing a null pointer when queue post Rx burst callback
> > removal while traffic is running.
> > See rte_eth_tx_burst function.
> >
> > Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev.h | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index d1a593ad1..35eb580ff 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> >  				     rx_pkts, nb_pkts);
> >
> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> > -		struct rte_eth_rxtx_callback *cb =
> > -				dev->post_rx_burst_cbs[queue_id];
> > -
> > +	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> > +	if (unlikely(cb != NULL)) {
> >  		do {
> >  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> >  						nb_pkts, cb->param);
> > --
> > 2.17.1
> While I don't have an issue with this fix, can you explain as to why this is a
> problem that needs to be fixed? Normally TOCTOU issues are flagged and
> fixed for external resources e.g. files, that can be modified between check
> and use, but this is just referencing internal data in the program itself,
> so I'm wondering what the risk is? From a security viewpoint if an attacker
> can modify the function pointers in our code, is it not already "game over"
> for keeping the running program safe?
> 

Right now RX/TX cb functions are not protected by any sync mechanism.
So while dataplane thread can do RX/TX control threads supposed to
be able to add/remove callbacks.
I am agree with Stephen here, we probably need either (volatile *)
or compiler_barrier() here.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-05 11:27     ` Ananyev, Konstantin
@ 2020-03-05 14:23       ` tgw_team
  2020-03-05 14:47       ` Liang, Ma
  1 sibling, 0 replies; 21+ messages in thread
From: tgw_team(腾讯网关团队) @ 2020-03-05 14:23 UTC (permalink / raw)
  To: Ananyev, Konstantin, Richardson, Bruce, ZY Qiu, Thomas Monjalon,
	Yigit, Ferruh, Andrew Rybchenko, dev


> > 
> > On Thu, Mar 05, 2020 at 01:33:49AM +0800, ZY Qiu wrote:
> > > When compiling with -O0,
> > > the compiler does not optimize two memory accesses into one.
> > > Leads to accessing a null pointer when queue post Rx burst callback
> > > removal while traffic is running.
> > > See rte_eth_tx_burst function.
> > >
> > > Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> > > ---
> > >  lib/librte_ethdev/rte_ethdev.h | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > > index d1a593ad1..35eb580ff 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> > >                                   rx_pkts, nb_pkts);
> > >
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > > -   if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> > > -           struct rte_eth_rxtx_callback *cb =
> > > -                           dev->post_rx_burst_cbs[queue_id];
> > > -
> > > +   struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> > > +   if (unlikely(cb != NULL)) {
> > >              do {
> > >                      nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> > >                                              nb_pkts, cb->param);
> > > --
> > > 2.17.1
> > While I don't have an issue with this fix, can you explain as to why this is a
> > problem that needs to be fixed? Normally TOCTOU issues are flagged and
> > fixed for external resources e.g. files, that can be modified between check
> > and use, but this is just referencing internal data in the program itself,
> > so I'm wondering what the risk is? From a security viewpoint if an attacker
> > can modify the function pointers in our code, is it not already "game over"
> > for keeping the running program safe?
> > 
> 
> Right now RX/TX cb functions are not protected by any sync mechanism.
> So while dataplane thread can do RX/TX control threads supposed to
> be able to add/remove callbacks.
> I am agree with Stephen here, we probably need either (volatile *)
> or compiler_barrier() here.

OK, I will fix rte_eth_rx_burst and rte_eth_tx_burst together in the next patch.

Thank you all for reply.

^_^



    

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-05 11:27     ` Ananyev, Konstantin
  2020-03-05 14:23       ` tgw_team
@ 2020-03-05 14:47       ` Liang, Ma
  2020-03-05 15:19         ` Ananyev, Konstantin
  1 sibling, 1 reply; 21+ messages in thread
From: Liang, Ma @ 2020-03-05 14:47 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Richardson, Bruce, ZY Qiu, Thomas Monjalon, Yigit, Ferruh,
	Andrew Rybchenko, dev, ZY Qiu

On 05 Mar 11:27, Ananyev, Konstantin wrote:
> 
> 
> > 
> > On Thu, Mar 05, 2020 at 01:33:49AM +0800, ZY Qiu wrote:
> > > When compiling with -O0,
> > > the compiler does not optimize two memory accesses into one.
> > > Leads to accessing a null pointer when queue post Rx burst callback
> > > removal while traffic is running.
> > > See rte_eth_tx_burst function.
> > >
> > > Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> > > ---
> > >  lib/librte_ethdev/rte_ethdev.h | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > > index d1a593ad1..35eb580ff 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> > >  				     rx_pkts, nb_pkts);
> > >
> > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > > -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> > > -		struct rte_eth_rxtx_callback *cb =
> > > -				dev->post_rx_burst_cbs[queue_id];
> > > -
> > > +	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> > > +	if (unlikely(cb != NULL)) {
> > >  		do {
> > >  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> > >  						nb_pkts, cb->param);
> > > --
> > > 2.17.1
> > While I don't have an issue with this fix, can you explain as to why this is a
> > problem that needs to be fixed? Normally TOCTOU issues are flagged and
> > fixed for external resources e.g. files, that can be modified between check
> > and use, but this is just referencing internal data in the program itself,
> > so I'm wondering what the risk is? From a security viewpoint if an attacker
> > can modify the function pointers in our code, is it not already "game over"
> > for keeping the running program safe?
> > 
> 
> Right now RX/TX cb functions are not protected by any sync mechanism.
> So while dataplane thread can do RX/TX control threads supposed to
> be able to add/remove callbacks.
> I am agree with Stephen here, we probably need either (volatile *)
> or compiler_barrier() here.
> 
> 
For my opinion,  
    the key question here is if the abstract layer code has to be thread safe or application
    developer look after thread safe of key data structure ?

        1. Single thread case :
           Current code has no issue even compiler behavior is different with -O0 or O3. 
           -O3 merge 2 loads into 1,  -O0 still use 2 loads. 

        2. Multiple thread case:
              As Konstantin said, there is no sync primitive to protect cb pointer at all. 
              Because of X86 64bit memory access is atomic, then, -O3 and -O0 will lead to totally different result.  
              I don’t think that's a fix because a Fix cannot depend on specific Arch is strong memory order or weak memory order. 

    Volatile or memory barrier may not fix this with a general style for multi-threads. 

    I will suggest add comment to clarify the scenario and let developer make decision.  
                    
Regards


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-05 14:47       ` Liang, Ma
@ 2020-03-05 15:19         ` Ananyev, Konstantin
  2020-03-05 15:42           ` Liang, Ma
  0 siblings, 1 reply; 21+ messages in thread
From: Ananyev, Konstantin @ 2020-03-05 15:19 UTC (permalink / raw)
  To: Ma, Liang J
  Cc: Richardson, Bruce, ZY Qiu, Thomas Monjalon, Yigit, Ferruh,
	Andrew Rybchenko, dev, ZY Qiu


> On 05 Mar 11:27, Ananyev, Konstantin wrote:
> >
> >
> > >
> > > On Thu, Mar 05, 2020 at 01:33:49AM +0800, ZY Qiu wrote:
> > > > When compiling with -O0,
> > > > the compiler does not optimize two memory accesses into one.
> > > > Leads to accessing a null pointer when queue post Rx burst callback
> > > > removal while traffic is running.
> > > > See rte_eth_tx_burst function.
> > > >
> > > > Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> > > > ---
> > > >  lib/librte_ethdev/rte_ethdev.h | 6 ++----
> > > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > > > index d1a593ad1..35eb580ff 100644
> > > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > > @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> > > >  				     rx_pkts, nb_pkts);
> > > >
> > > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > > > -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> > > > -		struct rte_eth_rxtx_callback *cb =
> > > > -				dev->post_rx_burst_cbs[queue_id];
> > > > -
> > > > +	struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> > > > +	if (unlikely(cb != NULL)) {
> > > >  		do {
> > > >  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> > > >  						nb_pkts, cb->param);
> > > > --
> > > > 2.17.1
> > > While I don't have an issue with this fix, can you explain as to why this is a
> > > problem that needs to be fixed? Normally TOCTOU issues are flagged and
> > > fixed for external resources e.g. files, that can be modified between check
> > > and use, but this is just referencing internal data in the program itself,
> > > so I'm wondering what the risk is? From a security viewpoint if an attacker
> > > can modify the function pointers in our code, is it not already "game over"
> > > for keeping the running program safe?
> > >
> >
> > Right now RX/TX cb functions are not protected by any sync mechanism.
> > So while dataplane thread can do RX/TX control threads supposed to
> > be able to add/remove callbacks.
> > I am agree with Stephen here, we probably need either (volatile *)
> > or compiler_barrier() here.
> >
> >
> For my opinion,
>     the key question here is if the abstract layer code has to be thread safe or application
>     developer look after thread safe of key data structure ?
> 
>         1. Single thread case :
>            Current code has no issue even compiler behavior is different with -O0 or O3.
>            -O3 merge 2 loads into 1,  -O0 still use 2 loads.
> 
>         2. Multiple thread case:
>               As Konstantin said, there is no sync primitive to protect cb pointer at all.
>               Because of X86 64bit memory access is atomic, then, -O3 and -O0 will lead to totally different result.
>               I don’t think that's a fix because a Fix cannot depend on specific Arch is strong memory order or weak memory order.
> 
>     Volatile or memory barrier may not fix this with a general style for multi-threads.

Can you elaborate why?
From my perspective compiler_barrier seems enough here.

> 
>     I will suggest add comment to clarify the scenario and let developer make decision.
> 
> Regards


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback
  2020-03-05 15:19         ` Ananyev, Konstantin
@ 2020-03-05 15:42           ` Liang, Ma
  0 siblings, 0 replies; 21+ messages in thread
From: Liang, Ma @ 2020-03-05 15:42 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Richardson, Bruce, ZY Qiu, Thomas Monjalon, Yigit, Ferruh,
	Andrew Rybchenko, dev, ZY Qiu

On 05 Mar 07:19, Ananyev, Konstantin wrote:
> 
> > On 05 Mar 11:27, Ananyev, Konstantin wrote:
> > >
> > >
> > > >
> > > > On Thu, Mar 05, 2020 at 01:33:49AM +0800, ZY Qiu wrote:
> > > > > When compiling with -O0,
> > > > > the compiler does not optimize two memory accesses into one.
> > > > > Leads to accessing a null pointer when queue post Rx burst callback
> > > > > removal while traffic is running.
> > > > > See rte_eth_tx_burst function.
> > > > >
> > > > > Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> > > > > ---
> > > > >  lib/librte_ethdev/rte_ethdev.h | 6 ++----
> > > > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > > > > index d1a593ad1..35eb580ff 100644
> > > > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > > > @@ -4388,10 +4388,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> > > > >       rx_pkts, nb_pkts);
> > > > >
> > > > >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > > > > -if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> > > > > -struct rte_eth_rxtx_callback *cb =
> > > > > -dev->post_rx_burst_cbs[queue_id];
> > > > > -
> > > > > +struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id];
> > > > > +if (unlikely(cb != NULL)) {
> > > > >  do {
> > > > >  nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> > > > >  nb_pkts, cb->param);
> > > > > --
> > > > > 2.17.1
> > > > While I don't have an issue with this fix, can you explain as to why this is a
> > > > problem that needs to be fixed? Normally TOCTOU issues are flagged and
> > > > fixed for external resources e.g. files, that can be modified between check
> > > > and use, but this is just referencing internal data in the program itself,
> > > > so I'm wondering what the risk is? From a security viewpoint if an attacker
> > > > can modify the function pointers in our code, is it not already "game over"
> > > > for keeping the running program safe?
> > > >
> > >
> > > Right now RX/TX cb functions are not protected by any sync mechanism.
> > > So while dataplane thread can do RX/TX control threads supposed to
> > > be able to add/remove callbacks.
> > > I am agree with Stephen here, we probably need either (volatile *)
> > > or compiler_barrier() here.
> > >
> > >
> > For my opinion,
> >     the key question here is if the abstract layer code has to be thread safe or application
> >     developer look after thread safe of key data structure ?
> >
> >         1. Single thread case :
> >            Current code has no issue even compiler behavior is different with -O0 or O3.
> >            -O3 merge 2 loads into 1,  -O0 still use 2 loads.
> >
> >         2. Multiple thread case:
> >               As Konstantin said, there is no sync primitive to protect cb pointer at all.
> >               Because of X86 64bit memory access is atomic, then, -O3 and -O0 will lead to totally different result.
> >               I don’t think that's a fix because a Fix cannot depend on specific Arch is strong memory order or weak memory order.
> >
> >     Volatile or memory barrier may not fix this with a general style for multi-threads.
> 
> Can you elaborate why?
> From my perspective compiler_barrier seems enough here.
I suspect rte_mb() here may not solve the problem for the weak memory order arch. 
> 
> >
> >     I will suggest add comment to clarify the scenario and let developer make decision.
> >
> > Regards
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
  2020-03-04 17:33 ` [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback ZY Qiu
  2020-03-04 17:56   ` Stephen Hemminger
  2020-03-05  9:19   ` Bruce Richardson
@ 2020-03-05 16:47   ` ZY Qiu
  2020-03-05 17:23     ` Jerin Jacob
  2020-03-11 12:22     ` Ananyev, Konstantin
  2 siblings, 2 replies; 21+ messages in thread
From: ZY Qiu @ 2020-03-05 16:47 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko
  Cc: dev, stephen, konstantin.ananyev, ZY Qiu

When compiling with -O0,
the compiler does not optimize two memory accesses into one.
Leads to accessing a null pointer when queue post Rx burst callback
removal while traffic is running.

Signed-off-by: ZY Qiu <tgw_team@tencent.com>
---
 lib/librte_ethdev/rte_ethdev.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad1..c46612e3e 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 				     rx_pkts, nb_pkts);
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
-		struct rte_eth_rxtx_callback *cb =
-				dev->post_rx_burst_cbs[queue_id];
+	struct rte_eth_rxtx_callback *volatile cb =
+			dev->post_rx_burst_cbs[queue_id];
 
+	if (unlikely(cb != NULL)) {
 		do {
 			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
 						nb_pkts, cb->param);
@@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
 #endif
 
 #ifdef RTE_ETHDEV_RXTX_CALLBACKS
-	struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
+	struct rte_eth_rxtx_callback *volatile cb =
+			dev->pre_tx_burst_cbs[queue_id];
 
 	if (unlikely(cb != NULL)) {
 		do {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
  2020-03-05 16:47   ` [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback ZY Qiu
@ 2020-03-05 17:23     ` Jerin Jacob
  2020-03-11 12:22     ` Ananyev, Konstantin
  1 sibling, 0 replies; 21+ messages in thread
From: Jerin Jacob @ 2020-03-05 17:23 UTC (permalink / raw)
  To: ZY Qiu
  Cc: Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, dpdk-dev,
	Stephen Hemminger, Ananyev, Konstantin, ZY Qiu

On Thu, Mar 5, 2020 at 10:17 PM ZY Qiu <quzeyao@gmail.com> wrote:
>
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when queue post Rx burst callback
> removal while traffic is running.
>
> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..c46612e3e 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>                                      rx_pkts, nb_pkts);
>
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -       if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -               struct rte_eth_rxtx_callback *cb =
> -                               dev->post_rx_burst_cbs[queue_id];
> +       struct rte_eth_rxtx_callback *volatile cb =
> +                       dev->post_rx_burst_cbs[queue_id];

Is adding rte_compiler_barrier() here without changing to volatile
solving the problem in your case?
If so, I prefer to change to compiler_barrier() as volatile may have a
performance impact on fast-path code.

>
> +       if (unlikely(cb != NULL)) {
>                 do {
>                         nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>                                                 nb_pkts, cb->param);
> @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
>  #endif
>
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -       struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
> +       struct rte_eth_rxtx_callback *volatile cb =
> +                       dev->pre_tx_burst_cbs[queue_id];
>
>         if (unlikely(cb != NULL)) {
>                 do {
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
  2020-03-05 16:47   ` [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback ZY Qiu
  2020-03-05 17:23     ` Jerin Jacob
@ 2020-03-11 12:22     ` Ananyev, Konstantin
  2020-03-11 12:26       ` Jerin Jacob
  1 sibling, 1 reply; 21+ messages in thread
From: Ananyev, Konstantin @ 2020-03-11 12:22 UTC (permalink / raw)
  To: ZY Qiu, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko
  Cc: dev, stephen, ZY Qiu



> -----Original Message-----
> From: ZY Qiu <quzeyao@gmail.com>
> Sent: Thursday, March 5, 2020 4:47 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; ZY Qiu
> <tgw_team@tencent.com>
> Subject: [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
> 
> When compiling with -O0,
> the compiler does not optimize two memory accesses into one.
> Leads to accessing a null pointer when queue post Rx burst callback
> removal while traffic is running.
> 
> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..c46612e3e 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>  				     rx_pkts, nb_pkts);
> 
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -	if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> -		struct rte_eth_rxtx_callback *cb =
> -				dev->post_rx_burst_cbs[queue_id];
> +	struct rte_eth_rxtx_callback *volatile cb =
> +			dev->post_rx_burst_cbs[queue_id];
> 
> +	if (unlikely(cb != NULL)) {
>  		do {
>  			nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>  						nb_pkts, cb->param);
> @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
>  #endif
> 
>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> -	struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
> +	struct rte_eth_rxtx_callback *volatile cb =
> +			dev->pre_tx_burst_cbs[queue_id];
> 
>  	if (unlikely(cb != NULL)) {
>  		do {
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
  2020-03-11 12:22     ` Ananyev, Konstantin
@ 2020-03-11 12:26       ` Jerin Jacob
  2020-10-01 15:14         ` Ferruh Yigit
  0 siblings, 1 reply; 21+ messages in thread
From: Jerin Jacob @ 2020-03-11 12:26 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: ZY Qiu, Thomas Monjalon, Yigit, Ferruh, Andrew Rybchenko, dev,
	stephen, ZY Qiu

On Wed, Mar 11, 2020 at 5:52 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: ZY Qiu <quzeyao@gmail.com>
> > Sent: Thursday, March 5, 2020 4:47 PM
> > To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
> > Cc: dev@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; ZY Qiu
> > <tgw_team@tencent.com>
> > Subject: [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
> >
> > When compiling with -O0,
> > the compiler does not optimize two memory accesses into one.
> > Leads to accessing a null pointer when queue post Rx burst callback
> > removal while traffic is running.
> >
> > Signed-off-by: ZY Qiu <tgw_team@tencent.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev.h | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index d1a593ad1..c46612e3e 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
> >                                    rx_pkts, nb_pkts);
> >
> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > -     if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
> > -             struct rte_eth_rxtx_callback *cb =
> > -                             dev->post_rx_burst_cbs[queue_id];
> > +     struct rte_eth_rxtx_callback *volatile cb =
> > +                     dev->post_rx_burst_cbs[queue_id];

I prefer to change to compiler_barrier() if it working as volatile may have a
performance impact on fast-path code.


> >
> > +     if (unlikely(cb != NULL)) {
> >               do {
> >                       nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
> >                                               nb_pkts, cb->param);
> > @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
> >  #endif
> >
> >  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
> > -     struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
> > +     struct rte_eth_rxtx_callback *volatile cb =
> > +                     dev->pre_tx_burst_cbs[queue_id];
> >
> >       if (unlikely(cb != NULL)) {
> >               do {
> > --
>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
> > 2.17.1
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
  2020-03-11 12:26       ` Jerin Jacob
@ 2020-10-01 15:14         ` Ferruh Yigit
  0 siblings, 0 replies; 21+ messages in thread
From: Ferruh Yigit @ 2020-10-01 15:14 UTC (permalink / raw)
  To: Jerin Jacob, Ananyev, Konstantin
  Cc: ZY Qiu, Thomas Monjalon, Andrew Rybchenko, dev, stephen, ZY Qiu

On 3/11/2020 12:26 PM, Jerin Jacob wrote:
> On Wed, Mar 11, 2020 at 5:52 PM Ananyev, Konstantin
> <konstantin.ananyev@intel.com> wrote:
>>
>>
>>
>>> -----Original Message-----
>>> From: ZY Qiu <quzeyao@gmail.com>
>>> Sent: Thursday, March 5, 2020 4:47 PM
>>> To: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
>>> Cc: dev@dpdk.org; stephen@networkplumber.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>; ZY Qiu
>>> <tgw_team@tencent.com>
>>> Subject: [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback
>>>
>>> When compiling with -O0,
>>> the compiler does not optimize two memory accesses into one.
>>> Leads to accessing a null pointer when queue post Rx burst callback
>>> removal while traffic is running.
>>>
>>> Signed-off-by: ZY Qiu <tgw_team@tencent.com>
>>> ---
>>>   lib/librte_ethdev/rte_ethdev.h | 9 +++++----
>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index d1a593ad1..c46612e3e 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -4388,10 +4388,10 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
>>>                                     rx_pkts, nb_pkts);
>>>
>>>   #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>>> -     if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {
>>> -             struct rte_eth_rxtx_callback *cb =
>>> -                             dev->post_rx_burst_cbs[queue_id];
>>> +     struct rte_eth_rxtx_callback *volatile cb =
>>> +                     dev->post_rx_burst_cbs[queue_id];
> 
> I prefer to change to compiler_barrier() if it working as volatile may have a
> performance impact on fast-path code.
> 

Hi ZY Qiu,

Did you able to test with compiler barrier, is it solving your problem?

If there is no update on the issue, I will mark it as rejected.

Thanks,
ferruh


> 
>>>
>>> +     if (unlikely(cb != NULL)) {
>>>                do {
>>>                        nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,
>>>                                                nb_pkts, cb->param);
>>> @@ -4652,7 +4652,8 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
>>>   #endif
>>>
>>>   #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>>> -     struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
>>> +     struct rte_eth_rxtx_callback *volatile cb =
>>> +                     dev->pre_tx_burst_cbs[queue_id];
>>>
>>>        if (unlikely(cb != NULL)) {
>>>                do {
>>> --
>>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>
>>> 2.17.1
>>


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-10-01 15:15 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04 14:05 [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback Tencent TGW team
2020-03-04 14:16 ` Andrew Rybchenko
     [not found]   ` <61a6b7d5533643d692c40dc0ab1a2cdc@tencent.com>
2020-03-04 16:26     ` tgw_team
2020-03-04 16:15 ` Stephen Hemminger
2020-03-04 16:38   ` [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail) tgw_team
2020-03-04 17:37     ` Stephen Hemminger
2020-03-04 17:44       ` [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback tgw_team
2020-03-04 17:33 ` [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback ZY Qiu
2020-03-04 17:56   ` Stephen Hemminger
2020-03-04 18:31     ` tgw_team
2020-03-05  9:19   ` Bruce Richardson
2020-03-05 11:27     ` Ananyev, Konstantin
2020-03-05 14:23       ` tgw_team
2020-03-05 14:47       ` Liang, Ma
2020-03-05 15:19         ` Ananyev, Konstantin
2020-03-05 15:42           ` Liang, Ma
2020-03-05 16:47   ` [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback ZY Qiu
2020-03-05 17:23     ` Jerin Jacob
2020-03-11 12:22     ` Ananyev, Konstantin
2020-03-11 12:26       ` Jerin Jacob
2020-10-01 15:14         ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).