On Thu, Oct 24, 2024 at 2:55 PM Morten Brørup <m...@smartsharesystems.com> wrote: > > > From: David Marchand [mailto:david.march...@redhat.com] > > Sent: Thursday, 24 October 2024 14.06 > > > > Caught by code review. > > > > Don't byte swap unconditionally (assuming that CPU is little endian is > > wrong). Instead, convert from big endian to cpu and vice versa. > > Yes looks like a bug. > I wonder if this PMD has more similar bugs... > grep bswap drivers/crypto/openssl/* says no. > > > @@ -110,9 +111,9 @@ ctr_inc(uint8_t *ctr) > > { > > uint64_t *ctr64 = (uint64_t *)ctr; > > > > - *ctr64 = __builtin_bswap64(*ctr64); > > + *ctr64 = rte_be_to_cpu_64(*ctr64); > > (*ctr64)++; > > - *ctr64 = __builtin_bswap64(*ctr64); > > + *ctr64 = rte_cpu_to_be_64(*ctr64); > > } > > But that's not all. > > There may be an alignment bug too; the way it is used in > process_openssl_cipher_des3ctr(), "ctr" is not guaranteed to be uint64_t > aligned. > > How about this instead: > > ctr_inc(void *ctr) > { > uint64_t ctr64 = rte_be_to_cpu_64(*(unaligned_uint64_t *)ctr); > ctr64++; > *(unaligned_uint64_t *)ctr = rte_cpu_to_be_64(ctr64); > } > > Or this: > > ctr_inc(void *ctr) > { > uint64_t ctr64; > > memcpy(&ctr64, ctr, sizeof(uint64_t)); > ctr64 = rte_be_to_cpu_64(ctr64); > ctr64++; > ctr64 = rte_cpu_to_be_64(ctr64); > memcpy(ctr, &ctr64, sizeof(uint64_t)); > } > > Or use a union in process_openssl_cipher_des3ctr() to ensure it's uint64_t > aligned.
Or declare ctr as a uint64_t in process_openssl_cipher_des3ctr directly, and remove this casting. -- David Marchand
