[v2] rte_ethdev: safer memory access by calling Rx callback
Checks
Commit Message
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
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.
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.
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
>
> 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.
> >
> > 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.
^_^
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
> 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
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
>
@@ -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);