> > > > > > 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. 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. > > > 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=DwIFA > > > 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. > > > > > > > > > > > > Thanks, > > > Anoob > > > > > > > -----Original Message----- > > > > From: dev <dev-boun...@dpdk.org> On Behalf Of Konstantin Ananyev > > > > Sent: Thursday, April 23, 2020 5:22 AM > > > > To: dev@dpdk.org > > > > Cc: akhil.go...@nxp.com; declan.dohe...@intel.com; Konstantin > > > > Ananyev <konstantin.anan...@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.anan...@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
