[dpdk-dev] [PATCH v4 04/10] lib: introduce ipsec library

Ananyev, Konstantin konstantin.ananyev at intel.com
Fri Dec 21 13:41:29 CET 2018



> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Friday, December 21, 2018 11:53 AM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
> Cc: Thomas Monjalon <thomas at monjalon.net>; Awal, Mohammad Abdul <mohammad.abdul.awal at 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.



More information about the dev mailing list