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

[Akhil Goyal <gakhil@marvell.com>]

> 
> On 25-Jul-24 5:47 AM, Akhil Goyal wrote:
> >> 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.
> >>
> > When a uint32 is used, there is no clarity whether it is in BE or LE.
> > So that is where the confusion comes.
> > For any new person, uint32 means it is in CPU byte order.
> > This patch tried to address that but it failed to address all the cases.
> > So my simple suggestion is instead of fixing the byte order at every place,
> > Lets just change the ipsec_sa->salt as rte_be32_t from uint32_t and revert this
> patch.
> > This will make things clear.
> > I am suggesting to convert this uint32_t to rte_be32_t for library as well for salt.
> > This change is not swapping anything, but making things explicitly clear that salt
> is in BE.
> 
> I agree with the suggestion of using rte_be32_t and reverting the patch.

Can you send a patch with both the changes?
> 
> (I still think it would be even better to use uint8_t[4] to reflect that
> it is a four byte value with no endianness but that's just IMHO, the
> important thing is to revert the patch that broke the functionality)
> 
That can be for next release in library.