diff mbox series

[v2] rte_ethdev: safer memory access by calling Rx callback

Message ID 20200304173349.26459-1-tgw_team@tencent.com (mailing list archive)
State Superseded, archived
Headers
Series [v2] rte_ethdev: safer memory access by calling Rx callback |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

ZY Qiu March 4, 2020, 5:33 p.m. UTC 
  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(-)

Comments

Stephen Hemminger March 4, 2020, 5:56 p.m. UTC | #1 
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.
tgw_team(腾讯网关团队) March 4, 2020, 6:31 p.m. UTC | #2 
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.
Bruce Richardson March 5, 2020, 9:19 a.m. UTC | #3 
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
Ananyev, Konstantin March 5, 2020, 11:27 a.m. UTC | #4 
> 
> 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.
tgw_team(腾讯网关团队) March 5, 2020, 2:23 p.m. UTC | #5 
> > 
> > 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.

^_^
Liang, Ma March 5, 2020, 2:47 p.m. UTC | #6 
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
Ananyev, Konstantin March 5, 2020, 3:19 p.m. UTC | #7 
> 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
Liang, Ma March 5, 2020, 3:42 p.m. UTC | #8 
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
>
diff mbox series

Patch

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