Hi Akhil,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Akhil Goyal <gak...@marvell.com>
> Sent: Thursday, May 18, 2023 1:10 PM
> To: Anoob Joseph <ano...@marvell.com>; Thomas Monjalon
> <tho...@monjalon.net>; Jerin Jacob Kollanukkaran <jer...@marvell.com>;
> Konstantin Ananyev <konstantin.v.anan...@yandex.ru>; Bernard
> Iremonger <bernard.iremon...@intel.com>
> Cc: Hemant Agrawal <hemant.agra...@nxp.com>; Mattias Rönnblom
> <mattias.ronnb...@ericsson.com>; Kiran Kumar Kokkilagadda
> <kirankum...@marvell.com>; Volodymyr Fialko <vfia...@marvell.com>;
> dev@dpdk.org; Olivier Matz <olivier.m...@6wind.com>
> Subject: RE: [PATCH v2 02/22] lib: add pdcp protocol
>
> Hi Anoob,
> > > > +static int
> > > > +pdcp_entity_priv_populate(struct entity_priv *en_priv, const
> > > > +struct
> > > > rte_pdcp_entity_conf *conf)
> > > > +{
> > > > + struct rte_crypto_sym_xform *c_xfrm, *a_xfrm;
> > > > + int ret;
> > > > +
> > > > + /**
> > > > + * flags.is_authenticated
> > > > + *
> > > > + * MAC-I would be added in case of control plane packets and
> > > > +when
> > > > authentication
> > > > + * transform is not NULL.
> > > > + */
> > > > +
> > > > + if (conf->pdcp_xfrm.domain ==
> > > > RTE_SECURITY_PDCP_MODE_CONTROL)
> > > > + en_priv->flags.is_authenticated = 1;
> > >
> > > This check should be added after getting the xfrm.
> > > If domain == control and a_xfrm is NULL, then it should be error, right?
> >
> > [Anoob] Lib PDCP would handle such cases. Even if a_xfrm is non NULL
> > but is NULL auth, it is lib PDCP which would add zeroized digest. And
> > a_xfrm == NULL is also treated as NULL auth generally. The comment
> > above this explains the same. Idea is to have lib PDCP handle all
> > possible cases rather than putting too much restrictions on both app &
> PMD.
>
> I believe there is a case in PDCP which allows no integrity as well.(LTE
> Uplane)
> In that case, a_xfrm is NULL is not equivalent to NULL auth.
> No integrity would mean no MAC-I.
[Anoob] The comment explains the logic. If it is control plane, then digest is
must and that case lib PDCP is trying to handle. Anyway, adding an extra error
check and returning error in that case is not a big deal. Will add that in the
appropriate place.
>
>
>
> > > > diff --git a/lib/pdcp/rte_pdcp.c b/lib/pdcp/rte_pdcp.c new file
> > > > mode
> > > > 100644 index 0000000000..8914548dbd
> > > > --- /dev/null
> > > > +++ b/lib/pdcp/rte_pdcp.c
> > > > @@ -0,0 +1,138 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright(C) 2023 Marvell.
> > > > + */
> > > > +
> > > > +#include <rte_errno.h>
> > > > +#include <rte_pdcp.h>
> > > > +#include <rte_malloc.h>
> > > > +
> > > > +#include "pdcp_crypto.h"
> > > > +#include "pdcp_entity.h"
> > > > +#include "pdcp_process.h"
> > > > +
> > > > +static int
> > > > +pdcp_entity_size_get(const struct rte_pdcp_entity_conf *conf) {
> > > > + int size;
> > > > +
> > > > + size = sizeof(struct rte_pdcp_entity) + sizeof(struct
> > > > +entity_priv);
> > > > +
> > > > + if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_DOWNLINK)
> > > > + size += sizeof(struct entity_priv_dl_part);
> > > > + else if (conf->pdcp_xfrm.pkt_dir == RTE_SECURITY_PDCP_UPLINK)
> > > > + size += sizeof(struct entity_priv_ul_part);
> > > > + else
> > > > + return -EINVAL;
> > > > +
> > > > + return RTE_ALIGN_CEIL(size, RTE_CACHE_LINE_SIZE); }
> > > > +
> > > > +struct rte_pdcp_entity *
> > > > +rte_pdcp_entity_establish(const struct rte_pdcp_entity_conf *conf) {
> > > > + struct rte_pdcp_entity *entity = NULL;
> > > > + struct entity_priv *en_priv;
> > > > + int ret, entity_size;
> > > > +
> > > > + if (conf == NULL || conf->cop_pool == NULL) {
> > > > + rte_errno = -EINVAL;
> > > > + return NULL;
> > > > + }
> > >
> > > errnos are normally set as positive values.
> >
> > [Anoob] I do not think so. I checked rte_ethdev.h, rte_flow.h etc and
> > all APIs are returning negative values in case of errors.
>
> Check again lib/ethdev/rte_ethdev.c
> rte_errno are set as positive values only and APIs return error numbers as
> negative values.
[Anoob] Okay. There are many APIs were this is not done correctly (check
rte_flow.h) . For lib PDCP additions, I'll have this handled. Some of these
conventions should be documented to avoid confusion.
>
>
> >
> > >
> > >
> > > > +
> > > > + if (conf->pdcp_xfrm.en_ordering || conf-
> > > > >pdcp_xfrm.remove_duplicates || conf->is_slrb ||
> > > > + conf->en_sec_offload) {
> > > > + rte_errno = -ENOTSUP;
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + /*
> > > > + * 6.3.2 PDCP SN
> > > > + * Length: 12 or 18 bits as indicated in table 6.3.2-1. The
> > > > length
> > > > +of the
> > > > PDCP SN is
> > > > + * configured by upper layers (pdcp-SN-SizeUL, pdcp-SN-SizeDL,
> > > > or
> > > > +sl-
> > > > PDCP-SN-Size in
> > > > + * TS 38.331 [3])
> > > > + */
> > > > + if ((conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_12)
> > > &&
> > > > + (conf->pdcp_xfrm.sn_size != RTE_SECURITY_PDCP_SN_SIZE_18)) {
> > > > + rte_errno = -ENOTSUP;
> > > > + return NULL;
> > > > + }
> > >
> > > Check for PDCP crypto algos may also be added.
> > > As only 4 cipher and 4 auth algos are supported in case of PDCP.
> >
> > [Anoob] Validation happens when we create session. Please check,
> > pdcp: add crypto session create and destroy
>
> Agreed, already acked that.
>
> >
> > >
> > > > +
> > > > + if (conf->pdcp_xfrm.hfn || conf->pdcp_xfrm.hfn_threshold) {
> > > > + rte_errno = -EINVAL;
> > > > + return NULL;
> > > > + }
> > >
> > > What is the reason to set errno as EINVAL when HFN is set?
> >
> > [Anoob] HFN is part of pdcp_xfrm which is defined in rte_security. Lib PDCP
> > allows user to specify complete 32 bit count value using
> > rte_pdcp_entity_conf.count. Since HFN is also used to construct 32 bit
> count
> > value, having two ways to set count would be misleading. Hence lib PDCP
> would
> > enforce that application does not set this value.
> >
> > >
> > > > +/**
> > > > + * PDCP entity configuration to be used for establishing an entity.
> > > > + */
> > > > +/* Structure rte_pdcp_entity_conf 8< */ struct rte_pdcp_entity_conf
> {
> > > > + /** PDCP transform for the entity. */
> > > > + struct rte_security_pdcp_xform pdcp_xfrm;
> > > > + /** Crypto transform applicable for the entity. */
> > > > + struct rte_crypto_sym_xform *crypto_xfrm;
> > > > + /** Mempool for crypto symmetric session. */
> > > > + struct rte_mempool *sess_mpool;
> > > > + /** Crypto op pool.*/
> > > > + struct rte_mempool *cop_pool;
> > > > + /**
> > > > + * 32 bit count value (HFN + SN) to be used for the first
> > > > packet.
> > > > + * pdcp_xfrm.hfn would be ignored as the HFN would be derived
> > > from
> > > > this value.
> > > > + */
> > >
> > > If the HFN is to be ignored, then why to add a check in entity establish
> and
> > > return EINVAL?
> > > It should be silently ignored in that case with a debug print at max.
> >
> > [Anoob] Explained above. Please check.
> >
> No it is not explained. Above comments says
> " pdcp_xfrm.hfn would be ignored as the HFN would be derived from this
> value"
>
> Here you are saying the field will be ignored.
> But while creating entity, it is returning EINVAL if hfn is provided.
[Anoob] Agreed. Will address in next version.
