> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.go...@nxp.com]
> Sent: Friday, December 21, 2018 11:53 AM
> To: Ananyev, Konstantin <konstantin.anan...@intel.com>; dev@dpdk.org
> Cc: Thomas Monjalon <tho...@monjalon.net>; Awal, Mohammad Abdul
> <mohammad.abdul.a...@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 04/10] lib: introduce ipsec library
>
>
>
> On 12/20/2018 7:36 PM, Ananyev, Konstantin wrote:
> >
> >>> diff --git a/lib/librte_ipsec/sa.c b/lib/librte_ipsec/sa.c
> >>> new file mode 100644
> >>> index 000000000..f927a82bf
> >>> --- /dev/null
> >>> +++ b/lib/librte_ipsec/sa.c
> >>> @@ -0,0 +1,327 @@
> >>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>> + * Copyright(c) 2018 Intel Corporation
> >>> + */
> >>> +
> >>> +#include <rte_ipsec_sa.h>
> >>> +#include <rte_esp.h>
> >>> +#include <rte_ip.h>
> >>> +#include <rte_errno.h>
> >>> +
> >>> +#include "sa.h"
> >>> +#include "ipsec_sqn.h"
> >>> +
> >>> +/* some helper structures */
> >>> +struct crypto_xform {
> >>> + struct rte_crypto_auth_xform *auth;
> >>> + struct rte_crypto_cipher_xform *cipher;
> >>> + struct rte_crypto_aead_xform *aead;
> >>> +};
> >> shouldn't this be union as aead cannot be with cipher and auth cases.
> > That's used internally to collect/analyze xforms provided by
> > prm->crypto_xform.
>
> >
> >
> >> extra line
> >>> +
> >>> +
> >>> +static int
> >>> +check_crypto_xform(struct crypto_xform *xform)
> >>> +{
> >>> + uintptr_t p;
> >>> +
> >>> + p = (uintptr_t)xform->auth | (uintptr_t)xform->cipher;
> >> what is the intent of this?
> > It is used below to check that if aead is present both cipher and auth
> > are not.
> >
> >>> +
> >>> + /* either aead or both auth and cipher should be not NULLs */
> >>> + if (xform->aead) {
> >>> + if (p)
> >>> + return -EINVAL;
> >>> + } else if (p == (uintptr_t)xform->auth) {
> >>> + return -EINVAL;
> >>> + }
> >> This function does not look good. It will miss the case of cipher only
> > Cipher only is not supported right now and I am not aware about plans
> > to support it in future.
> > If someone would like to add cipher onl,then yes he/she probably would
> > have to update this function.
> I know that cipher_only is not supported and nobody will support it in
> case of ipsec.
> My point is if somebody gives only auth or only cipher xform, then this
> function would not be able to detect that case and will not return error.
fill_crypto_xform() (the function below) will detect it and return an error:
+ if (xf->type == RTE_CRYPTO_SYM_XFORM_AUTH) {
+ if (xform->auth != NULL)
+ return -EINVAL;
>
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int
> >>> +fill_crypto_xform(struct crypto_xform *xform,
> >>> + const struct rte_ipsec_sa_prm *prm)
> >>> +{
> >>> + struct rte_crypto_sym_xform *xf;
> >>> +
> >>> + memset(xform, 0, sizeof(*xform));
> >>> +
> >>> + for (xf = prm->crypto_xform; xf != NULL; xf = xf->next) {
> >>> + if (xf->type == RTE_CRYPTO_SYM_XFORM_AUTH) {
> >>> + if (xform->auth != NULL)
> >>> + return -EINVAL;
> >>> + xform->auth = &xf->auth;
> >>> + } else if (xf->type == RTE_CRYPTO_SYM_XFORM_CIPHER) {
> >>> + if (xform->cipher != NULL)
> >>> + return -EINVAL;
> >>> + xform->cipher = &xf->cipher;
> >>> + } else if (xf->type == RTE_CRYPTO_SYM_XFORM_AEAD) {
> >>> + if (xform->aead != NULL)
> >>> + return -EINVAL;
> >>> + xform->aead = &xf->aead;
> >>> + } else
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + return check_crypto_xform(xform);
> >>> +}
> >> how is this function handling the inbound and outbound cases.
> >> In inbound first xform is auth and then cipher.
> >> In outbound first is cipher and then auth. I think this should be
> >> checked in the lib.
> > Interesting, I didn't know about such limitation.
> > My understanding was that the any order (<auth,cipher>, <cipher,auth>)
> > for both inbound and outbound is acceptable.
> > Is that order restriction is documented somewhere?
> /**
> * Symmetric crypto transform structure.
> *
> * This is used to specify the crypto transforms required, multiple
> transforms
> * can be chained together to specify a chain transforms such as
> authentication
> * then cipher, or cipher then authentication. Each transform structure can
> * hold a single transform, the type field is used to specify which
> transform
> * is contained within the union
> */
> struct rte_crypto_sym_xform {
Yes, I read this but I don't see where it says that order of xforms implicitly
defines order of operations for that session within crypto-dev.
Or is it just me?
I don't mind to add extra check here, just want to be sure it is really required
for crypto PMD to work correctly.
>
> This is not a limitation, this is how it is designed to handle 2 cases
> of crypto - auth then cipher and cipher then auth.
>
Ok, if you sure it is a valid check - I'll add it.
>
> >> Here for loop should not be there, as there would be at max only 2 xforms.
> >>> +
> >>> +uint64_t __rte_experimental
> >>> +rte_ipsec_sa_type(const struct rte_ipsec_sa *sa)
> >>> +{
> >>> + return sa->type;
> >>> +}
> >>> +
> >>> +static int32_t
> >>> +ipsec_sa_size(uint32_t wsz, uint64_t type, uint32_t *nb_bucket)
> >>> +{
> >>> + uint32_t n, sz;
> >>> +
> >>> + n = 0;
> >>> + if (wsz != 0 && (type & RTE_IPSEC_SATP_DIR_MASK) ==
> >>> + RTE_IPSEC_SATP_DIR_IB)
> >>> + n = replay_num_bucket(wsz);
> >>> +
> >>> + if (n > WINDOW_BUCKET_MAX)
> >>> + return -EINVAL;
> >>> +
> >>> + *nb_bucket = n;
> >>> +
> >>> + sz = rsn_size(n);
> >>> + sz += sizeof(struct rte_ipsec_sa);
> >>> + return sz;
> >>> +}
> >>> +
> >>> +void __rte_experimental
> >>> +rte_ipsec_sa_fini(struct rte_ipsec_sa *sa)
> >>> +{
> >>> + memset(sa, 0, sa->size);
> >>> +}
> >> Where is the memory of "sa" getting initialized?
> > Not sure I understand your question...
> > Do you mean we missed memset(sa, 0, size)
> > in rte_ipsec_sa_init()?
> Sorry I did not ask the correct question, I was asking - where it is
> allocated?
> Is it application's responsibility?
Yes, it is an application responsibility to allocate the memory buffer.
But looking at code again - actually we did miss memset() here,
will update.
