On 24-Jul-24 2:04 PM, Akhil Goyal wrote:
On 24-Jul-24 12:20 PM, Akhil Goyal wrote:
On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
Hi all,

This patch breaks ipsec tests with ipsec-secgw:


./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
...
ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
dst=192.168.31.14,
sz=1
    test IPv4 trs_aesctr_sha1 finished with status 1
ERROR  test trs_aesctr_sha1 FAILED

The patch seems to be correct.
Please check endianness in the PMD you are testing.
In my opinion salt should not be affected by endianness and it should be
used as it is in the key parameter. I think the patch is wrong to make
it CPU endianness dependent before being passed to the PMDs, any PMD
that needs the endianness swapped should do it in the PMD code. Indeed
it's passed around as a 32 bit integer but it's not used as such, and
when it's actually used it should be evaluated as a byte array.

As per the rfc, it should be treated as byte order(i.e. big endian).
But here the problem is we treat it as uint32_t which makes it CPU endian
when stored in ipsec_sa struct.
The keys are stored as an array of uint8_t, so keys are stored in byte order(Big
endian).
So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
So that when it is stored in PMD/HW, and we convert it from uint32_t to uint_8
*, there wont be issue.

RFC treats it as a "four octet value" - there is no endianness until
it's treated like an integer, which it never is. Even if it code it's
being stored and passed as an unsigned 32bit integer it is never
evaluated as such so its endianness doesn't matter.
The endianness matters the moment it is stored as uint32_t in ipsec_sa
It means the value is stored in CPU endianness in that integer unless it is 
specified.

What matters is that the four byte value in the key ends up in the memory in the same order, and that was always the case before the patch, regardless of the endianness of the CPU because load and store operations are not affected by endianness. With the patch the same four bytes from the configuration file will be stored differently in memory depending on the CPU. There is no need to fix the endianness of the salt, just as there is no need to fix the byte order of the key itself.


Now looking at the code again, I see the patch is incomplete for the case of 
lookaside crypto
Where the salt is copied as cnt_blk in the mbuf priv without conversion.

So, this patch can be reverted and a simple fix can be added to mark ipsec_sa-> 
salt as rte_be32_t
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index a83fd2283b..1fe6b97168 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -117,7 +117,7 @@ struct __rte_cache_aligned ipsec_sa {
         uint32_t spi;
         struct cdev_qp *cqp[RTE_MAX_LCORE];
         uint64_t seq;
-       uint32_t salt;
+       rte_be32_t salt;
         uint32_t fallback_sessions;
         enum rte_crypto_cipher_algorithm cipher_algo;
         enum rte_crypto_auth_algorithm auth_algo;

Can you verify and send the patch?
And this may be updated in cryptodev and security lib as well in next release.


I agree that we should have it everywhere as "uint8_t salt[4]" but that
implies API changes and it doesn't change how the bytes are stored, so
the patch will still be wrong.


On 03/07/2024 18:58, Akhil Goyal wrote:





                -----Original Message-----
                From: Akhil Goyal <gak...@marvell.com>
<mailto:gak...@marvell.com>
                Sent: Friday, March 15, 2024 12:42 AM
                To: Akhil Goyal <gak...@marvell.com>
<mailto:gak...@marvell.com> ; Chaoyong He
                <chaoyong...@corigine.com>
<mailto:chaoyong...@corigine.com> ; dev@dpdk.org
<mailto:dev@dpdk.org>
                Cc: oss-driv...@corigine.com <mailto:oss-
driv...@corigine.com> ; Shihong Wang <shihong.w...@corigine.com>
<mailto:shihong.w...@corigine.com> ;
                sta...@dpdk.org <mailto:sta...@dpdk.org>
                Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
SA salt
                endianness problem


                        Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
secgw: fix SA salt
                        endianness problem


                                From: Shihong Wang
<shihong.w...@corigine.com> <mailto:shihong.w...@corigine.com>

                                The SA salt of struct ipsec_sa is a CPU-endian
u32 variable, but it’s
                                value is stored in an array of encryption or
authentication keys
                                according to big-endian. So it maybe need to
convert the endianness
                                order to ensure that the value assigned to the
SA salt is CPU-endian.

                                Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
initialize SA salt")
                                Fixes: 9413c3901f31 ("examples/ipsec-secgw:
support additional algorithms")
                                Fixes: 501e9c226adf ("examples/ipsec-secgw:
add AEAD parameters")
                                Cc: sta...@dpdk.org <mailto:sta...@dpdk.org>

                                Signed-off-by: Shihong Wang
<shihong.w...@corigine.com> <mailto:shihong.w...@corigine.com>
                                Reviewed-by: Chaoyong He
<chaoyong...@corigine.com> <mailto:chaoyong...@corigine.com>


                        Acked-by: Akhil Goyal <gak...@marvell.com>
<mailto:gak...@marvell.com>

                        Applied to dpdk-next-crypto


                The patch is pulled back from dpdk-next-crypto.
                This change may cause all the PMDs to fail these cases.
                Would need acks from PMDs.


        Applied to dpdk-next-crypto
        No update from PMD owners.
        Applying it before RC2 so that we have time for fixes if needed.


--
Regards,
Vladimir

Reply via email to