From: Akhil Goyal <akhil.goyal@nxp.com>
To: Konstantin Ananyev <konstantin.ananyev@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix SPD no-match is misinterpreted
Date: Fri, 29 Mar 2019 10:53:52 +0000 [thread overview]
Message-ID: <aea63dff-be62-e850-abb3-e52bb04a9de7@nxp.com> (raw)
Message-ID: <20190329105352.x0rRF0hL5tsnvJvLsYLlIbHkqcA_C-CPCqQB2BdhCm4@z> (raw)
In-Reply-To: <20190328124733.25580-1-konstantin.ananyev@intel.com>
Hi Konstantin,
On 3/28/2019 6:17 PM, Konstantin Ananyev wrote:
> acl_classify() returns zero value when no matching rule was found.
> Currently ipsec-secgw treats it as a valid SPI value, though it has
> to discard such packets.
> Error could be easily observed by sending outbound unmatched packets,
> user will see something like that in the log:
> IPSEC: No cryptodev: core 7, cipher_algo 0, auth_algo 0, aead_algo 0
>
> To fix it we need to treat packets with zero result from acl_classify()
> as invalid ones. Also we can change DISCARD and BYPASS values to
> simplify checks and save some extra space for valid SPI values.
spi value =0 is invalid but zero result may have a valid packet.
consider a case:
SPI = 128 or 256 or 512 and so on => sa_idx = 0 and result will come as
zero, and this would be a valid packet.
I see that the sa_idx calculation logic is not correct in first place.
There will be multiple spi values for same sa_idx which is not correct.
So we have 2 issues here:
1. result = 0, means sa_idx =0 which may be correct, but as you said if
acl_classify fails, it also return 0.
2. SPI values which are IPSEC_SA_MAX_ENTRIES apart will have same sa_idx
and will keep on overwriting the previous ones.
So I believe the fix in this patch is not enough to resolve these
issues. It will work on some values and will break on other values of spi.
-Akhil
>
> Fixes: 906257e965b7 ("examples/ipsec-secgw: support IPv6")
> Fixes: 2a5106af132b ("examples/ipsec-secgw: fix corner case for SPI value")
> Cc: stable@dpdk.org
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> examples/ipsec-secgw/ipsec-secgw.c | 12 ++++++------
> examples/ipsec-secgw/ipsec.h | 6 ++----
> examples/ipsec-secgw/sp4.c | 11 ++++++++---
> examples/ipsec-secgw/sp6.c | 11 ++++++++---
> 4 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/examples/ipsec-secgw/ipsec-secgw.c b/examples/ipsec-secgw/ipsec-secgw.c
> index ffbd00b08..59e084234 100644
> --- a/examples/ipsec-secgw/ipsec-secgw.c
> +++ b/examples/ipsec-secgw/ipsec-secgw.c
> @@ -438,11 +438,11 @@ inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa, struct traffic_type *ip,
> for (i = 0; i < ip->num; i++) {
> m = ip->pkts[i];
> res = ip->res[i];
> - if (res & BYPASS) {
> + if (res == BYPASS) {
> ip->pkts[j++] = m;
> continue;
> }
> - if (res & DISCARD) {
> + if (res == DISCARD) {
> rte_pktmbuf_free(m);
> continue;
> }
> @@ -453,7 +453,7 @@ inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa, struct traffic_type *ip,
> continue;
> }
>
> - sa_idx = ip->res[i] & PROTECT_MASK;
> + sa_idx = ip->res[i];
> if (sa_idx >= IPSEC_SA_MAX_ENTRIES ||
> !inbound_sa_check(sa, m, sa_idx)) {
> rte_pktmbuf_free(m);
> @@ -541,10 +541,10 @@ outbound_sp(struct sp_ctx *sp, struct traffic_type *ip,
> j = 0;
> for (i = 0; i < ip->num; i++) {
> m = ip->pkts[i];
> - sa_idx = ip->res[i] & PROTECT_MASK;
> - if (ip->res[i] & DISCARD)
> + sa_idx = ip->res[i];
> + if (sa_idx == DISCARD)
> rte_pktmbuf_free(m);
> - else if (ip->res[i] & BYPASS)
> + else if (sa_idx == BYPASS)
> ip->pkts[j++] = m;
> else if (sa_idx < IPSEC_SA_MAX_ENTRIES) {
> ipsec->res[ipsec->num] = sa_idx;
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index 99f49d65f..44daf384b 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -41,10 +41,8 @@
> #define SPI2IDX(spi) (spi & (IPSEC_SA_MAX_ENTRIES - 1))
> #define INVALID_SPI (0)
>
> -#define DISCARD (0x80000000)
> -#define BYPASS (0x40000000)
> -#define PROTECT_MASK (0x3fffffff)
> -#define PROTECT(sa_idx) (SPI2IDX(sa_idx) & PROTECT_MASK) /* SA idx 30 bits */
> +#define DISCARD INVALID_SPI
> +#define BYPASS UINT32_MAX
>
> #define IPSEC_XFORM_MAX 2
>
> diff --git a/examples/ipsec-secgw/sp4.c b/examples/ipsec-secgw/sp4.c
> index d1dc64bad..bfaddc52e 100644
> --- a/examples/ipsec-secgw/sp4.c
> +++ b/examples/ipsec-secgw/sp4.c
> @@ -99,6 +99,7 @@ parse_sp4_tokens(char **tokens, uint32_t n_tokens,
>
> uint32_t *ri = NULL; /* rule index */
> uint32_t ti = 0; /* token index */
> + uint32_t tv;
>
> uint32_t esp_p = 0;
> uint32_t protect_p = 0;
> @@ -169,8 +170,12 @@ parse_sp4_tokens(char **tokens, uint32_t n_tokens,
> if (status->status < 0)
> return;
>
> - rule_ipv4->data.userdata =
> - PROTECT(atoi(tokens[ti]));
> + tv = atoi(tokens[ti]);
> + APP_CHECK(tv != DISCARD && tv != BYPASS, status,
> + "invalid SPI: %s", tokens[ti]);
> + if (status->status < 0)
> + return;
> + rule_ipv4->data.userdata = tv;
>
> protect_p = 1;
> continue;
> @@ -523,7 +528,7 @@ sp4_spi_present(uint32_t spi, int inbound)
> }
>
> for (i = 0; i != num; i++) {
> - if (acr[i].data.userdata == PROTECT(spi))
> + if (acr[i].data.userdata == spi)
> return i;
> }
>
> diff --git a/examples/ipsec-secgw/sp6.c b/examples/ipsec-secgw/sp6.c
> index e67d85aaf..b7fcf7c16 100644
> --- a/examples/ipsec-secgw/sp6.c
> +++ b/examples/ipsec-secgw/sp6.c
> @@ -130,6 +130,7 @@ parse_sp6_tokens(char **tokens, uint32_t n_tokens,
>
> uint32_t *ri = NULL; /* rule index */
> uint32_t ti = 0; /* token index */
> + uint32_t tv;
>
> uint32_t esp_p = 0;
> uint32_t protect_p = 0;
> @@ -202,8 +203,12 @@ parse_sp6_tokens(char **tokens, uint32_t n_tokens,
> if (status->status < 0)
> return;
>
> - rule_ipv6->data.userdata =
> - PROTECT(atoi(tokens[ti]));
> + tv = atoi(tokens[ti]);
> + APP_CHECK(tv != DISCARD && tv != BYPASS, status,
> + "invalid SPI: %s", tokens[ti]);
> + if (status->status < 0)
> + return;
> + rule_ipv6->data.userdata = tv;
>
> protect_p = 1;
> continue;
> @@ -637,7 +642,7 @@ sp6_spi_present(uint32_t spi, int inbound)
> }
>
> for (i = 0; i != num; i++) {
> - if (acr[i].data.userdata == PROTECT(spi))
> + if (acr[i].data.userdata == spi)
> return i;
> }
>
next prev parent reply other threads:[~2019-03-29 10:53 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-28 12:47 Konstantin Ananyev
2019-03-28 12:47 ` Konstantin Ananyev
2019-03-29 10:53 ` Akhil Goyal [this message]
2019-03-29 10:53 ` Akhil Goyal
2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-29 18:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
2019-03-30 11:22 ` Ananyev, Konstantin
2019-04-04 12:16 ` Ananyev, Konstantin
2019-04-04 12:16 ` Ananyev, Konstantin
2019-04-04 12:13 ` [dpdk-dev] [PATCH v2] " Konstantin Ananyev
2019-04-04 12:13 ` Konstantin Ananyev
2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-04 18:39 ` Zhang, Roy Fan
2019-04-23 12:58 ` Akhil Goyal
2019-04-23 12:58 ` Akhil Goyal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aea63dff-be62-e850-abb3-e52bb04a9de7@nxp.com \
--to=akhil.goyal@nxp.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).