[dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.

[dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.

[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

Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.

[Andrew Rybchenko]

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?

Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.

[Stephen Hemminger]

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

Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.

[tgw_team(腾讯网关团队)]

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.

Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail)

[tgw_team(腾讯网关团队)]

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

    

[dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[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

Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.(Internet mail)

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH] rte_ethdev: fix unsafe memory access by calling RX callback.

[tgw_team(腾讯网关团队)]

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.

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[Stephen Hemminger]

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.

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[tgw_team(腾讯网关团队)]

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.


    

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[Bruce Richardson]

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

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[Ananyev, Konstantin]

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

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[tgw_team(腾讯网关团队)]

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

^_^



    

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[Liang, Ma]

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

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[Ananyev, Konstantin]

> 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

Re: [dpdk-dev] [PATCH v2] rte_ethdev: safer memory access by calling Rx callback

[Liang, Ma]

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
> 

[dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback

[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

Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback

[Jerin Jacob]

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
>

Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback

[Ananyev, Konstantin]

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

Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback

[Jerin Jacob]

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
>

Re: [dpdk-dev] [PATCH v3] rte_ethdev: safer memory access by calling Rx/Tx callback

[Ferruh Yigit]

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