Hi Konstantin, Please see inline.
Thanks, Anoob > -----Original Message----- > From: Ananyev, Konstantin <konstantin.anan...@intel.com> > Sent: Thursday, April 23, 2020 1:24 PM > To: Anoob Joseph <ano...@marvell.com>; dev@dpdk.org > Cc: akhil.go...@nxp.com; Doherty, Declan <declan.dohe...@intel.com>; > techbo...@dpdk.org > Subject: [EXT] RE: [dpdk-dev] [PATCH] security: fix crash at accessing non- > implemented ops > > External Email > > ---------------------------------------------------------------------- > > > > Hi Konstantin, > > > > 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. > 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. > 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? > 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. > > > > > > > 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
