From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id D5AD9A05D3 for ; Sun, 19 May 2019 16:47:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A224A28F3; Sun, 19 May 2019 16:47:15 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id B7FDB28F3 for ; Sun, 19 May 2019 16:47:13 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 May 2019 07:47:12 -0700 X-ExtLoop1: 1 Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by fmsmga008.fm.intel.com with ESMTP; 19 May 2019 07:47:10 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.155]) by IRSMSX152.ger.corp.intel.com ([169.254.6.31]) with mapi id 14.03.0415.000; Sun, 19 May 2019 15:47:10 +0100 From: "Ananyev, Konstantin" To: Lukasz Bartosik CC: "dev@dpdk.org" , "anoobj@marvell.com" Thread-Topic: [PATCH] ipsec: include high order bytes of esn in pkt len Thread-Index: AQHU/2Tjbnoxxn6xQkWOyA+XCdWcPKZyfUrQ Date: Sun, 19 May 2019 14:47:09 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580161635D0D@irsmsx105.ger.corp.intel.com> References: <1556636155-26299-1-git-send-email-lbartosik@marvell.com> In-Reply-To: <1556636155-26299-1-git-send-email-lbartosik@marvell.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzA3MzFlM2UtZGI0Yy00Y2ViLWExMmQtZjQ5MWM2YmFkMzY2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiMVQ5ZlRIT3RhR3JwbkZ2anRveDhKdkFBamNCNFBJYmhjRG05aHh1bXo4OEdPZWd0U3VscVRiWUN3Rkg1aWF6dyJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] ipsec: include high order bytes of esn in pkt len X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Lukasz, Thanks for clarifications. Looks good in general. Few small comments below. Konstantin =20 > When esn is used then high-order 32 bits are included in ICV > calculation however are not transmitted. Update packet length > to be consistent with auth data offset and length before crypto > operation. High-order 32 bits of esn will be removed from packet > length in crypto post processing. >=20 > Change-Id: I5ba50e341059a8d6a5e4ce7c626dfe7b9173740b > Signed-off-by: Lukasz Bartosik > --- > lib/librte_ipsec/esp_inb.c | 56 +++++++++++++++++++++++++++++++++++++--= ------ > lib/librte_ipsec/esp_outb.c | 31 +++++++++++++++++++++++++ > lib/librte_ipsec/sa.c | 4 ++-- > lib/librte_ipsec/sa.h | 8 +++++++ > 4 files changed, 87 insertions(+), 12 deletions(-) >=20 > diff --git a/lib/librte_ipsec/esp_inb.c b/lib/librte_ipsec/esp_inb.c > index 4e0e12a..eb899e3 100644 > --- a/lib/librte_ipsec/esp_inb.c > +++ b/lib/librte_ipsec/esp_inb.c > @@ -16,7 +16,8 @@ > #include "pad.h" >=20 > typedef uint16_t (*esp_inb_process_t)(const struct rte_ipsec_sa *sa, > - struct rte_mbuf *mb[], uint32_t sqn[], uint32_t dr[], uint16_t num); > + struct rte_mbuf *mb[], uint32_t sqn[], uint32_t dr[], uint16_t num, > + uint8_t is_inline); >=20 > /* > * helper function to fill crypto_sym op for cipher+auth algorithms. > @@ -181,6 +182,15 @@ inb_pkt_prepare(const struct rte_ipsec_sa *sa, const= struct replay_sqn *rsn, > icv->va =3D rte_pktmbuf_mtod_offset(ml, void *, icv_ofs); > icv->pa =3D rte_pktmbuf_iova_offset(ml, icv_ofs); >=20 > + /* > + * if esn is used then high-order 32 bits are also used in ICV > + * calculation but are not transmitted, update packet length > + * to be consistent with auth data length and offset, this will > + * be subtracted from packet length in post crypto processing Here and in several other comments below - you repeat basically the same th= ing. Seems a bit excessive. I suppose just to put in in one place, or probably e= ven in patch description will be enough. > + */ > + mb->pkt_len +=3D sa->sqh_len; > + ml->data_len +=3D sa->sqh_len; > + > inb_pkt_xprepare(sa, sqn, icv); > return plen; > } > @@ -373,14 +383,20 @@ tun_process_step3(struct rte_mbuf *mb, uint64_t txo= f_msk, uint64_t txof_val) > */ > static inline uint16_t > tun_process(const struct rte_ipsec_sa *sa, struct rte_mbuf *mb[], > - uint32_t sqn[], uint32_t dr[], uint16_t num) > + uint32_t sqn[], uint32_t dr[], uint16_t num, uint8_t is_inline) > { > uint32_t adj, i, k, tl; > uint32_t hl[num]; > struct esp_tail espt[num]; > struct rte_mbuf *ml[num]; >=20 > - const uint32_t tlen =3D sa->icv_len + sizeof(espt[0]); > + /* > + * remove high-order 32 bits of esn from packet length > + * which was added before crypto processing, this doesn't > + * apply to inline case > + */ This comment seems a bit misleading, as we have remove not only sqh, but also icv, espt, padding. > + const uint32_t tlen =3D sa->icv_len + sizeof(espt[0]) + > + (is_inline ? 0 : sa->sqh_len); > const uint32_t cofs =3D sa->ctp.cipher.offset; >=20 > /* > @@ -420,7 +436,7 @@ tun_process(const struct rte_ipsec_sa *sa, struct rte= _mbuf *mb[], > */ > static inline uint16_t > trs_process(const struct rte_ipsec_sa *sa, struct rte_mbuf *mb[], > - uint32_t sqn[], uint32_t dr[], uint16_t num) > + uint32_t sqn[], uint32_t dr[], uint16_t num, uint8_t is_inline) > { > char *np; > uint32_t i, k, l2, tl; > @@ -428,7 +444,13 @@ trs_process(const struct rte_ipsec_sa *sa, struct rt= e_mbuf *mb[], > struct esp_tail espt[num]; > struct rte_mbuf *ml[num]; >=20 > - const uint32_t tlen =3D sa->icv_len + sizeof(espt[0]); > + /* > + * remove high-order 32 bits of esn from packet length > + * which was added before crypto processing, this doesn't > + * apply to inline case > + */ > + const uint32_t tlen =3D sa->icv_len + sizeof(espt[0]) + > + (is_inline ? 0 : sa->sqh_len); > const uint32_t cofs =3D sa->ctp.cipher.offset; >=20 > /* > @@ -496,8 +518,8 @@ esp_inb_rsn_update(struct rte_ipsec_sa *sa, const uin= t32_t sqn[], > * process group of ESP inbound packets. > */ > static inline uint16_t > -esp_inb_pkt_process(const struct rte_ipsec_session *ss, > - struct rte_mbuf *mb[], uint16_t num, esp_inb_process_t process) > +esp_inb_pkt_process(const struct rte_ipsec_session *ss, struct rte_mbuf = *mb[], > + uint16_t num, uint8_t is_inline, esp_inb_process_t process) > { > uint32_t k, n; > struct rte_ipsec_sa *sa; > @@ -507,7 +529,7 @@ esp_inb_pkt_process(const struct rte_ipsec_session *s= s, > sa =3D ss->sa; >=20 > /* process packets, extract seq numbers */ > - k =3D process(sa, mb, sqn, dr, num); > + k =3D process(sa, mb, sqn, dr, num, is_inline); >=20 > /* handle unprocessed mbufs */ > if (k !=3D num && k !=3D 0) > @@ -533,7 +555,14 @@ uint16_t > esp_inb_tun_pkt_process(const struct rte_ipsec_session *ss, > struct rte_mbuf *mb[], uint16_t num) > { > - return esp_inb_pkt_process(ss, mb, num, tun_process); > + return esp_inb_pkt_process(ss, mb, num, 0, tun_process); To make things a bit cleaner, can I suggest the following: 1. make esp_inb_pkt_process() take as a parameters:=20 const struct rte_ipsec_sa *sa (instead of const struct rte_ipsec_sessio= n *ss) uint32_t sqh_len instead of is_inlne So here it would become: sa =3D ss->sa; return esp_inb_pkt_process(ss, mb, num, sa->sqh_len, tun_process); For inline it would be: sa =3D ss->sa; return esp_inb_pkt_process(ss, mb, num, 0, tun_process); > +} > + > +uint16_t > +inline_inb_tun_pkt_process(const struct rte_ipsec_session *ss, > + struct rte_mbuf *mb[], uint16_t num) > +{ > + return esp_inb_pkt_process(ss, mb, num, 1, tun_process); > } >=20 > /* > @@ -543,5 +572,12 @@ uint16_t > esp_inb_trs_pkt_process(const struct rte_ipsec_session *ss, > struct rte_mbuf *mb[], uint16_t num) > { > - return esp_inb_pkt_process(ss, mb, num, trs_process); > + return esp_inb_pkt_process(ss, mb, num, 0, trs_process); > +} > + > +uint16_t > +inline_inb_trs_pkt_process(const struct rte_ipsec_session *ss, > + struct rte_mbuf *mb[], uint16_t num) > +{ > + return esp_inb_pkt_process(ss, mb, num, 1, trs_process); > } > diff --git a/lib/librte_ipsec/esp_outb.c b/lib/librte_ipsec/esp_outb.c > index c798bc4..71a595e 100644 > --- a/lib/librte_ipsec/esp_outb.c > +++ b/lib/librte_ipsec/esp_outb.c > @@ -221,6 +221,7 @@ esp_outb_tun_prepare(const struct rte_ipsec_session *= ss, struct rte_mbuf *mb[], > uint32_t i, k, n; > uint64_t sqn; > rte_be64_t sqc; > + struct rte_mbuf *ml; > struct rte_ipsec_sa *sa; > struct rte_cryptodev_sym_session *cs; > union sym_op_data icv; > @@ -246,6 +247,19 @@ esp_outb_tun_prepare(const struct rte_ipsec_session = *ss, struct rte_mbuf *mb[], >=20 > /* success, setup crypto op */ > if (rc >=3D 0) { > + /* > + * if esn is used then high-order 32 bits are also > + * used in ICV calculation but are not transmitted, > + * update packet length to be consistent with auth > + * data length and offset, this will be subtracted > + * from packet length in post crypto processing > + */ > + if (sa->sqh_len) { > + mb[i]->pkt_len +=3D sa->sqh_len; > + ml =3D rte_pktmbuf_lastseg(mb[i]); > + ml->data_len +=3D sa->sqh_len; > + } That means we have to go through our 'next' list once again. Seems suboptimal. I think would be better to make outb_tun_pkt_prepare() to take sqh_len as e= xtra parameter. Then inside it we can just: tlen =3D pdlen + sa->icv_len + sqh_len;=20 ... if (tlen + sa->aad_len > rte_pktmbuf_tailroom(ml)) return -ENOSPC;=20 ... ml->data_len +=3D tlen; mb->pkt_len +=3D tlen; ... pdofs +=3D pdlen + sqh_len; Same for transport mode. > + > outb_pkt_xprepare(sa, sqc, &icv); > lksd_none_cop_prepare(cop[k], cs, mb[i]); > outb_cop_prepare(cop[k], sa, iv, &icv, 0, rc); > @@ -356,6 +370,7 @@ esp_outb_trs_prepare(const struct rte_ipsec_session *= ss, struct rte_mbuf *mb[], > uint32_t i, k, n, l2, l3; > uint64_t sqn; > rte_be64_t sqc; > + struct rte_mbuf *ml; > struct rte_ipsec_sa *sa; > struct rte_cryptodev_sym_session *cs; > union sym_op_data icv; > @@ -384,6 +399,19 @@ esp_outb_trs_prepare(const struct rte_ipsec_session = *ss, struct rte_mbuf *mb[], >=20 > /* success, setup crypto op */ > if (rc >=3D 0) { > + /* > + * if esn is used then high-order 32 bits are also > + * used in ICV calculation but are not transmitted, > + * update packet length to be consistent with auth > + * data length and offset, this will be subtracted > + * from packet length in post crypto processing > + */ > + if (sa->sqh_len) { > + mb[i]->pkt_len +=3D sa->sqh_len; > + ml =3D rte_pktmbuf_lastseg(mb[i]); > + ml->data_len +=3D sa->sqh_len; > + } > + > outb_pkt_xprepare(sa, sqc, &icv); > lksd_none_cop_prepare(cop[k], cs, mb[i]); > outb_cop_prepare(cop[k], sa, iv, &icv, l2 + l3, rc); > @@ -425,6 +453,9 @@ esp_outb_sqh_process(const struct rte_ipsec_session *= ss, struct rte_mbuf *mb[], > for (i =3D 0; i !=3D num; i++) { > if ((mb[i]->ol_flags & PKT_RX_SEC_OFFLOAD_FAILED) =3D=3D 0) { > ml =3D rte_pktmbuf_lastseg(mb[i]); > + /* remove high-order 32 bits of esn from packet len */ > + mb[i]->pkt_len -=3D sa->sqh_len; > + ml->data_len -=3D sa->sqh_len; > icv =3D rte_pktmbuf_mtod_offset(ml, void *, > ml->data_len - icv_len); > remove_sqh(icv, icv_len); > diff --git a/lib/librte_ipsec/sa.c b/lib/librte_ipsec/sa.c > index 846e317..ff01358 100644 > --- a/lib/librte_ipsec/sa.c > +++ b/lib/librte_ipsec/sa.c > @@ -610,10 +610,10 @@ inline_crypto_pkt_func_select(const struct rte_ipse= c_sa *sa, > switch (sa->type & msk) { > case (RTE_IPSEC_SATP_DIR_IB | RTE_IPSEC_SATP_MODE_TUNLV4): > case (RTE_IPSEC_SATP_DIR_IB | RTE_IPSEC_SATP_MODE_TUNLV6): > - pf->process =3D esp_inb_tun_pkt_process; > + pf->process =3D inline_inb_tun_pkt_process; > break; > case (RTE_IPSEC_SATP_DIR_IB | RTE_IPSEC_SATP_MODE_TRANS): > - pf->process =3D esp_inb_trs_pkt_process; > + pf->process =3D inline_inb_trs_pkt_process; > break; > case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV4): > case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV6): > diff --git a/lib/librte_ipsec/sa.h b/lib/librte_ipsec/sa.h > index ffb5fb4..20c0a65 100644 > --- a/lib/librte_ipsec/sa.h > +++ b/lib/librte_ipsec/sa.h > @@ -143,9 +143,17 @@ esp_inb_tun_pkt_process(const struct rte_ipsec_sessi= on *ss, > struct rte_mbuf *mb[], uint16_t num); >=20 > uint16_t > +inline_inb_tun_pkt_process(const struct rte_ipsec_session *ss, > + struct rte_mbuf *mb[], uint16_t num); > + > +uint16_t > esp_inb_trs_pkt_process(const struct rte_ipsec_session *ss, > struct rte_mbuf *mb[], uint16_t num); >=20 > +uint16_t > +inline_inb_trs_pkt_process(const struct rte_ipsec_session *ss, > + struct rte_mbuf *mb[], uint16_t num); > + > /* outbound processing */ >=20 > uint16_t > -- > 2.7.4