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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

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