Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, April 23, 2020 4:25 PM
> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org; Lukasz
> Wojciechowski <l.wojciechow@partner.samsung.com>
> Cc: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>
> Subject: [EXT] RE: [dpdk-dev] [PATCH] security: fix crash at accessing non-
> implemented ops
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> > > >
> > > > These are data path ops and so it will be better if we can avoid
> > > > such checks in
> > > the datapath. The same is done in ethdev also.
> > >
> > > AFAIK,  get_userdata is an *optional* dev-ops function that can be
> > > used by data- path.
> > > So far there was no strict requirement for the rte_security PMDs to
> > > *always* implement it.
> >
> > [Anoob] I don't think DPDK categorizes dev-ops as *optional* and *always*. If
> yes, can you point me?
> 
> > My understanding is, all ops are optional. For example, I could
> > implement a crypto PMD which is doing packet delivery only via event device
> (using crypto adapter). So dequeue op will not be implemented in that case and
> DPDK spec allows it.
> 
> Your PMD can have enqueue_burst/dequeue_burst as NOP, but you still have  to
> provide valid function pointers:
> they are stored inside crypto_dev structure itself and will be called
> unconditionally (without any extra checking) by
> rte_cryptodev_enqueue_burst/rte_cryptodev_dequeue_burst.

[Anoob] I think there is a basic misunderstanding here. You are treating unconditional calls as mandatory implementations. If that is the case rte_eth_tx_burst() & rte_eth_rx_burst() shouldn't check for function pointers even when DEBUG is enabled.

static inline uint16_t
rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
		 struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
{
	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
	uint16_t nb_rx;

#ifdef RTE_LIBRTE_ETHDEV_DEBUG
	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
	RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);

	if (queue_id >= dev->data->nb_rx_queues) {
		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", queue_id);
		return 0;
	}
#endif
	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
				     rx_pkts, nb_pkts);

From my view point, function pointer checks and argument checks are required in every API for stability. But having such checks in the datapath adversely affects the performance. And for cases where function pointers are not set, application would get one crash in the first run. And that can be debugged after having the required options enabled.
 
> For all other calls (both data and control path) there is a check that actual
> function pointer is a valid one.
> Same story for eth dev: pkt_rx_burst/pkt_tx_burst and rest of dev-ops.
> 
> > > So what you guys did is a silent change of public API behaviour.
> >
> > [Anoob] I believe Lukasz had submitted 3 or 4 revisions and it was all in the ML.
> RTE_DEBUG was suggested by Thomas I guess.
> 
> I believe it is not a right procedure to change existing behaviour of rte_security
> framework.
> I think you have to communicate clear and loudly in advance (at least one
> release in advance).
> Plus RTE_DEBUG has nothing to do with changing non-debug behaviour.
> 
> > > As result ixgbe, (and probably some others rte_security PMDs)
> > > stopped working properly.
> >
> > [Anoob] set_pkt_metadata() is the only one of interest to IXGBE. And I
> > believe the function is implemented as well. So what exactly is the concern?
> 
> Check that ops->get_userdata is a valid function pointer will be compiled out.
> So PMDs that don't implement this function will crash in
> rte_security_get_userdata().
> In our particular case - ixgbe.
> Same story with  rte_security_set_pkt_metadata() - see the patch.

[Anoob] But ixgbe doesn't implement inline protocol which is the primary consumer of this API (rte_security_get_userdata()). So what is the trouble? 

Also, application is expected to call rte_security_set_pkt_metadata() only on devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a PMD states it needs MDATA but fails to register a function pointer for doing the same, it is a control path problem. Checking for that in the datapath is an overkill.

> 
> >
> > > I don't see any point in these changes, but if you'd like to do
> > > that, at least our usual procedure has to be followed:
> > > 1. Send and RFC to get an agreement with rte_security PMDs
> > > maintainers (one release ahead) 2. send a deprecation note (one
> > > release ahead) 3. change the behaviour of the public API 4. update
> > > release notes
> > >
> > > AFAIK 1), 2), 4) wasn't done.
> > > So I think right now we need to revert original behaviour.
> > >
> > > >
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_
> > > > dpdk
> > > > _v20.02_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4372&d=DwIFAg
> > > > &c=n
> > > > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > > WYLn1v9SyTMrT5EQqh2TU&m=
> > > > 6ObfSanVVuHOsiqVlWxXsFWi-
> > > 2XNp76HCOX0vbUfma4&s=jDVyDDEILmgY1Yb9ZBswBVbn
> > > > 8FpZuQI5ukH_osmtUiI&e=
> > > >
> > > > Datapath functions in cryptodev (enqueue/dequeue) doesn't even
> > > > have such
> > > checks.
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_
> > > > dpdk
> > > > _v20.02_source_lib_librte-5Fcryptodev_rte-5Fcryptodev.h-23L962&d=D
> > > > wIFA
> > > > g&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > > WYLn1v9SyTMrT5EQqh2
> > > > TU&m=6ObfSanVVuHOsiqVlWxXsFWi-
> > > 2XNp76HCOX0vbUfma4&s=LEWQOKs0r2Im_zL95VI
> > > > df4kQ2Pu0iRHV9Co2J1gsNBE&e=
> > >
> > > That's a different story:
> > > rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions
> > > that have to be implemented by each  ethdev/cryptodev API.
> >
> > [Anoob] I couldn't find any reference stating that way. If you can
> > point me, I can update that to include datapath ops required for inline protocol
> processing.
> 
> Look at the code.

[Anoob] Code is not conclusive for this as I've explained above for rte_eth_rx/tx_burst() case.
 
> 
> >
> > >
> > > >
> > > >
> > > > Thanks,
> > > > Anoob
> > > >
> > > > > -----Original Message-----
> > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> > > > > Sent: Thursday, April 23, 2020 5:22 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin
> > > > > Ananyev <konstantin.ananyev@intel.com>
> > > > > Subject: [dpdk-dev] [PATCH] security: fix crash at accessing
> > > > > non-implemented ops
> > > > >
> > > > > Valid checks for optional function pointers inside dev-ops were
> > > > > disabled by undefined macro.
> > > > >
> > > > > Fixes: b6ee98547847 ("security: fix verification of parameters")
> > > > >
> > > > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > > ---
> > > > >  lib/librte_security/rte_security.c | 4 ----
> > > > >  1 file changed, 4 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_security/rte_security.c
> > > > > b/lib/librte_security/rte_security.c
> > > > > index d475b0977..b65430ce2 100644
> > > > > --- a/lib/librte_security/rte_security.c
> > > > > +++ b/lib/librte_security/rte_security.c
> > > > > @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct
> > > > > rte_security_ctx *instance,
> > > > >  			      struct rte_security_session *sess,
> > > > >  			      struct rte_mbuf *m, void *params)  { -#ifdef
> > > RTE_DEBUG
> > > > >  	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> > > > > EINVAL,
> > > > >  			-ENOTSUP);
> > > > >  	RTE_PTR_OR_ERR_RET(sess, -EINVAL); -#endif
> > > > >  	return instance->ops->set_pkt_metadata(instance->device,
> > > > >  					       sess, m, params);
> > > > >  }
> > > > > @@ -121,9 +119,7 @@ rte_security_get_userdata(struct
> > > > > rte_security_ctx *instance, uint64_t md)  {
> > > > >  	void *userdata = NULL;
> > > > >
> > > > > -#ifdef RTE_DEBUG
> > > > >  	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> > > > > NULL); -#endif
> > > > >  	if (instance->ops->get_userdata(instance->device, md, &userdata))
> > > > >  		return NULL;
> > > > >
> > > > > --
> > > > > 2.17.1