[12/12] test/ipsec: fix build with GCC 12
Checks
Commit Message
GCC 12 raises the following warning:
In function ‘_mm256_loadu_si256’,
inlined from ‘rte_mov32’ at
../lib/eal/x86/include/rte_memcpy.h:319:9,
inlined from ‘rte_mov128’ at
../lib/eal/x86/include/rte_memcpy.h:344:2,
inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:438:4,
inlined from ‘rte_memcpy’ at
../lib/eal/x86/include/rte_memcpy.h:882:10,
inlined from ‘setup_test_string.constprop’ at
../app/test/test_ipsec.c:572:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript ‘__m256i_u[3]’ is partly outside array bounds of
‘const char[108]’ [-Werror=array-bounds]
929 | return *__P;
| ^~~~
../app/test/test_ipsec.c: In function ‘setup_test_string.constprop’:
../app/test/test_ipsec.c:539:12: note: at offset 96 into object
‘null_plain_data’ of size 108
539 | const char null_plain_data[] =
| ^~~~~~~~~~~~~~~
Split copy request into copies of string lengths and remove unused
blocksize.
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/test_ipsec.c | 48 ++++++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 19 deletions(-)
Comments
Hi David,
On 18/05/2022 11:16, David Marchand wrote:
> GCC 12 raises the following warning:
>
> In function ‘_mm256_loadu_si256’,
> inlined from ‘rte_mov32’ at
> ../lib/eal/x86/include/rte_memcpy.h:319:9,
> inlined from ‘rte_mov128’ at
> ../lib/eal/x86/include/rte_memcpy.h:344:2,
> inlined from ‘rte_memcpy_generic’ at
> ../lib/eal/x86/include/rte_memcpy.h:438:4,
> inlined from ‘rte_memcpy’ at
> ../lib/eal/x86/include/rte_memcpy.h:882:10,
> inlined from ‘setup_test_string.constprop’ at
> ../app/test/test_ipsec.c:572:4:
> /usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
> array subscript ‘__m256i_u[3]’ is partly outside array bounds of
> ‘const char[108]’ [-Werror=array-bounds]
> 929 | return *__P;
> | ^~~~
> ../app/test/test_ipsec.c: In function ‘setup_test_string.constprop’:
> ../app/test/test_ipsec.c:539:12: note: at offset 96 into object
> ‘null_plain_data’ of size 108
> 539 | const char null_plain_data[] =
> | ^~~~~~~~~~~~~~~
>
> Split copy request into copies of string lengths and remove unused
> blocksize.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> app/test/test_ipsec.c | 48 ++++++++++++++++++++++++++-----------------
> 1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/app/test/test_ipsec.c b/app/test/test_ipsec.c
> index 8da025bf66..d7455fd021 100644
> --- a/app/test/test_ipsec.c
> +++ b/app/test/test_ipsec.c
> @@ -554,24 +554,28 @@ struct rte_ipv4_hdr ipv4_outer = {
> };
>
> static struct rte_mbuf *
> -setup_test_string(struct rte_mempool *mpool,
> - const char *string, size_t len, uint8_t blocksize)
> +setup_test_string(struct rte_mempool *mpool, const char *string,
> + size_t string_len, size_t len)
> {
> struct rte_mbuf *m = rte_pktmbuf_alloc(mpool);
> - size_t t_len = len - (blocksize ? (len % blocksize) : 0);
>
> if (m) {
> memset(m->buf_addr, 0, m->buf_len);
> - char *dst = rte_pktmbuf_append(m, t_len);
> + char *dst = rte_pktmbuf_append(m, len);
>
> if (!dst) {
> rte_pktmbuf_free(m);
> return NULL;
> }
> - if (string != NULL)
> - rte_memcpy(dst, string, t_len);
> - else
> - memset(dst, 0, t_len);
> + if (string != NULL) {
> + size_t off;
> +
> + for (off = 0; off + string_len < len; off += string_len)
I think it should be off + string_len <= len here, because otherwise, if
len is a multiple of string_len, the last ret_memcpy (after this loop)
will copy 0 bytes.
> + rte_memcpy(&dst[off], string, string_len);
> + rte_memcpy(&dst[off], string, len % string_len);
> + } else {
> + memset(dst, 0, len);
> + }
> }
>
> return m;
> @@ -1365,7 +1369,8 @@ test_ipsec_crypto_outb_burst_null_null(int i)
> /* Generate input mbuf data */
> for (j = 0; j < num_pkts && rc == 0; j++) {
> ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
> - null_plain_data, test_cfg[i].pkt_sz, 0);
> + null_plain_data, sizeof(null_plain_data),
> + test_cfg[i].pkt_sz);
> if (ut_params->ibuf[j] == NULL)
> rc = TEST_FAILED;
> else {
> @@ -1483,7 +1488,8 @@ test_ipsec_inline_crypto_inb_burst_null_null(int i)
> /* Generate test mbuf data */
> ut_params->obuf[j] = setup_test_string(
> ts_params->mbuf_pool,
> - null_plain_data, test_cfg[i].pkt_sz, 0);
> + null_plain_data, sizeof(null_plain_data),
> + test_cfg[i].pkt_sz);
> if (ut_params->obuf[j] == NULL)
> rc = TEST_FAILED;
> }
> @@ -1551,16 +1557,17 @@ test_ipsec_inline_proto_inb_burst_null_null(int i)
>
> /* Generate inbound mbuf data */
> for (j = 0; j < num_pkts && rc == 0; j++) {
> - ut_params->ibuf[j] = setup_test_string(
> - ts_params->mbuf_pool,
> - null_plain_data, test_cfg[i].pkt_sz, 0);
> + ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
> + null_plain_data, sizeof(null_plain_data),
> + test_cfg[i].pkt_sz);
> if (ut_params->ibuf[j] == NULL)
> rc = TEST_FAILED;
> else {
> /* Generate test mbuf data */
> ut_params->obuf[j] = setup_test_string(
> ts_params->mbuf_pool,
> - null_plain_data, test_cfg[i].pkt_sz, 0);
> + null_plain_data, sizeof(null_plain_data),
> + test_cfg[i].pkt_sz);
> if (ut_params->obuf[j] == NULL)
> rc = TEST_FAILED;
> }
> @@ -1660,7 +1667,8 @@ test_ipsec_inline_crypto_outb_burst_null_null(int i)
> /* Generate test mbuf data */
> for (j = 0; j < num_pkts && rc == 0; j++) {
> ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
> - null_plain_data, test_cfg[i].pkt_sz, 0);
> + null_plain_data, sizeof(null_plain_data),
> + test_cfg[i].pkt_sz);
> if (ut_params->ibuf[0] == NULL)
> rc = TEST_FAILED;
>
> @@ -1738,15 +1746,16 @@ test_ipsec_inline_proto_outb_burst_null_null(int i)
> /* Generate test mbuf data */
> for (j = 0; j < num_pkts && rc == 0; j++) {
> ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
> - null_plain_data, test_cfg[i].pkt_sz, 0);
> + null_plain_data, sizeof(null_plain_data),
> + test_cfg[i].pkt_sz);
> if (ut_params->ibuf[0] == NULL)
> rc = TEST_FAILED;
>
> if (rc == 0) {
> /* Generate test tunneled mbuf data for comparison */
> ut_params->obuf[j] = setup_test_string(
> - ts_params->mbuf_pool,
> - null_plain_data, test_cfg[i].pkt_sz, 0);
> + ts_params->mbuf_pool, null_plain_data,
> + sizeof(null_plain_data), test_cfg[i].pkt_sz);
> if (ut_params->obuf[j] == NULL)
> rc = TEST_FAILED;
> }
> @@ -1815,7 +1824,8 @@ test_ipsec_lksd_proto_inb_burst_null_null(int i)
> for (j = 0; j < num_pkts && rc == 0; j++) {
> /* packet with sequence number 0 is invalid */
> ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
> - null_encrypted_data, test_cfg[i].pkt_sz, 0);
> + null_encrypted_data, sizeof(null_encrypted_data),
> + test_cfg[i].pkt_sz);
> if (ut_params->ibuf[j] == NULL)
> rc = TEST_FAILED;
> }
Hello Vladimir,
On Thu, Jun 2, 2022 at 8:42 PM Medvedkin, Vladimir
<vladimir.medvedkin@intel.com> wrote:
> > if (!dst) {
> > rte_pktmbuf_free(m);
> > return NULL;
> > }
> > - if (string != NULL)
> > - rte_memcpy(dst, string, t_len);
> > - else
> > - memset(dst, 0, t_len);
> > + if (string != NULL) {
> > + size_t off;
> > +
> > + for (off = 0; off + string_len < len; off += string_len)
>
> I think it should be off + string_len <= len here, because otherwise, if
> len is a multiple of string_len, the last ret_memcpy (after this loop)
> will copy 0 bytes.
Changing to off + string_len <= len would trigger an oob access to dst
(by one extra byte)?
Otoh, I don't think it is an issue to have a 0-length call to rte_memcpy.
>
> > + rte_memcpy(&dst[off], string, string_len);
> > + rte_memcpy(&dst[off], string, len % string_len);
On Fri, Jun 03, 2022 at 09:45:45AM +0200, David Marchand wrote:
> Hello Vladimir,
>
> On Thu, Jun 2, 2022 at 8:42 PM Medvedkin, Vladimir
> <vladimir.medvedkin@intel.com> wrote:
> > > if (!dst) {
> > > rte_pktmbuf_free(m);
> > > return NULL;
> > > }
> > > - if (string != NULL)
> > > - rte_memcpy(dst, string, t_len);
> > > - else
> > > - memset(dst, 0, t_len);
> > > + if (string != NULL) {
> > > + size_t off;
> > > +
> > > + for (off = 0; off + string_len < len; off += string_len)
> >
> > I think it should be off + string_len <= len here, because otherwise, if
> > len is a multiple of string_len, the last ret_memcpy (after this loop)
> > will copy 0 bytes.
>
> Changing to off + string_len <= len would trigger an oob access to dst
> (by one extra byte)?
> Otoh, I don't think it is an issue to have a 0-length call to rte_memcpy.
>
Given this is test code, do we need rte_memcpy for performance over regular
libc memcpy? Does fixing the warning become any easier or clearer if libc
memcpy is used?
On Fri, Jun 3, 2022 at 9:56 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Jun 03, 2022 at 09:45:45AM +0200, David Marchand wrote:
> > Hello Vladimir,
> >
> > On Thu, Jun 2, 2022 at 8:42 PM Medvedkin, Vladimir
> > <vladimir.medvedkin@intel.com> wrote:
> > > > if (!dst) {
> > > > rte_pktmbuf_free(m);
> > > > return NULL;
> > > > }
> > > > - if (string != NULL)
> > > > - rte_memcpy(dst, string, t_len);
> > > > - else
> > > > - memset(dst, 0, t_len);
> > > > + if (string != NULL) {
> > > > + size_t off;
> > > > +
> > > > + for (off = 0; off + string_len < len; off += string_len)
> > >
> > > I think it should be off + string_len <= len here, because otherwise, if
> > > len is a multiple of string_len, the last ret_memcpy (after this loop)
> > > will copy 0 bytes.
> >
> > Changing to off + string_len <= len would trigger an oob access to dst
> > (by one extra byte)?
> > Otoh, I don't think it is an issue to have a 0-length call to rte_memcpy.
> >
> Given this is test code, do we need rte_memcpy for performance over regular
> libc memcpy? Does fixing the warning become any easier or clearer if libc
> memcpy is used?
There was a similar proposal in vhost/crypto code.
I am not a fan to switching to libc memcpy.
We would be waiving a potential issue in rte_memcpy itself (which
could also be a problem in how gcc understands this inlined code) or
in the rte_memcpy caller code.
Here, gcc is probably too picky.
No path currently leads to oob access on the src string.
Adding a simple hint (see simplified hunk below) seems to help gcc enough:
@@ -554,12 +554,14 @@ struct rte_ipv4_hdr ipv4_outer = {
};
static struct rte_mbuf *
-setup_test_string(struct rte_mempool *mpool,
- const char *string, size_t len, uint8_t blocksize)
+setup_test_string(struct rte_mempool *mpool, const char *string,
+ size_t string_len, size_t len, uint8_t blocksize)
{
struct rte_mbuf *m = rte_pktmbuf_alloc(mpool);
size_t t_len = len - (blocksize ? (len % blocksize) : 0);
+ RTE_VERIFY(len <= string_len);
+
if (m) {
memset(m->buf_addr, 0, m->buf_len);
Hi David,
On 03/06/2022 10:41, David Marchand wrote:
> On Fri, Jun 3, 2022 at 9:56 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
>>
>> On Fri, Jun 03, 2022 at 09:45:45AM +0200, David Marchand wrote:
>>> Hello Vladimir,
>>>
>>> On Thu, Jun 2, 2022 at 8:42 PM Medvedkin, Vladimir
>>> <vladimir.medvedkin@intel.com> wrote:
>>>>> if (!dst) {
>>>>> rte_pktmbuf_free(m);
>>>>> return NULL;
>>>>> }
>>>>> - if (string != NULL)
>>>>> - rte_memcpy(dst, string, t_len);
>>>>> - else
>>>>> - memset(dst, 0, t_len);
>>>>> + if (string != NULL) {
>>>>> + size_t off;
>>>>> +
>>>>> + for (off = 0; off + string_len < len; off += string_len)
>>>>
>>>> I think it should be off + string_len <= len here, because otherwise, if
>>>> len is a multiple of string_len, the last ret_memcpy (after this loop)
>>>> will copy 0 bytes.
>>>
>>> Changing to off + string_len <= len would trigger an oob access to dst
>>> (by one extra byte)?
>>> Otoh, I don't think it is an issue to have a 0-length call to rte_memcpy.
>>>
The problem here is that if, for example, string_len is 8 bytes and len
is 16, then it will write only 8 bytes.
>> Given this is test code, do we need rte_memcpy for performance over regular
>> libc memcpy? Does fixing the warning become any easier or clearer if libc
>> memcpy is used?
>
> There was a similar proposal in vhost/crypto code.
> I am not a fan to switching to libc memcpy.
> We would be waiving a potential issue in rte_memcpy itself (which
> could also be a problem in how gcc understands this inlined code) or
> in the rte_memcpy caller code.
>
> Here, gcc is probably too picky.
> No path currently leads to oob access on the src string.
>
> Adding a simple hint (see simplified hunk below) seems to help gcc enough:
>
> @@ -554,12 +554,14 @@ struct rte_ipv4_hdr ipv4_outer = {
> };
>
> static struct rte_mbuf *
> -setup_test_string(struct rte_mempool *mpool,
> - const char *string, size_t len, uint8_t blocksize)
> +setup_test_string(struct rte_mempool *mpool, const char *string,
> + size_t string_len, size_t len, uint8_t blocksize)
> {
> struct rte_mbuf *m = rte_pktmbuf_alloc(mpool);
> size_t t_len = len - (blocksize ? (len % blocksize) : 0);
>
> + RTE_VERIFY(len <= string_len);
> +
RTE_VERIFY looks better here to make picky GCC happy.
>
> if (m) {
> memset(m->buf_addr, 0, m->buf_len);
>
>
On Wed, 18 May 2022 12:16:57 +0200
David Marchand <david.marchand@redhat.com> wrote:
> GCC 12 raises the following warning:
>
> In function ‘_mm256_loadu_si256’,
> inlined from ‘rte_mov32’ at
> ../lib/eal/x86/include/rte_memcpy.h:319:9,
> inlined from ‘rte_mov128’ at
> ../lib/eal/x86/include/rte_memcpy.h:344:2,
> inlined from ‘rte_memcpy_generic’ at
> ../lib/eal/x86/include/rte_memcpy.h:438:4,
> inlined from ‘rte_memcpy’ at
> ../lib/eal/x86/include/rte_memcpy.h:882:10,
> inlined from ‘setup_test_string.constprop’ at
> ../app/test/test_ipsec.c:572:4:
> /usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
> array subscript ‘__m256i_u[3]’ is partly outside array bounds of
> ‘const char[108]’ [-Werror=array-bounds]
> 929 | return *__P;
> | ^~~~
> ../app/test/test_ipsec.c: In function ‘setup_test_string.constprop’:
> ../app/test/test_ipsec.c:539:12: note: at offset 96 into object
> ‘null_plain_data’ of size 108
> 539 | const char null_plain_data[] =
> | ^~~~~~~~~~~~~~~
>
> Split copy request into copies of string lengths and remove unused
> blocksize.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
Why is test code for ipsec bother with using rte_memcpy at all.
Instead global replace rte_memcpy() with memcpy() for the whole test.
@@ -554,24 +554,28 @@ struct rte_ipv4_hdr ipv4_outer = {
};
static struct rte_mbuf *
-setup_test_string(struct rte_mempool *mpool,
- const char *string, size_t len, uint8_t blocksize)
+setup_test_string(struct rte_mempool *mpool, const char *string,
+ size_t string_len, size_t len)
{
struct rte_mbuf *m = rte_pktmbuf_alloc(mpool);
- size_t t_len = len - (blocksize ? (len % blocksize) : 0);
if (m) {
memset(m->buf_addr, 0, m->buf_len);
- char *dst = rte_pktmbuf_append(m, t_len);
+ char *dst = rte_pktmbuf_append(m, len);
if (!dst) {
rte_pktmbuf_free(m);
return NULL;
}
- if (string != NULL)
- rte_memcpy(dst, string, t_len);
- else
- memset(dst, 0, t_len);
+ if (string != NULL) {
+ size_t off;
+
+ for (off = 0; off + string_len < len; off += string_len)
+ rte_memcpy(&dst[off], string, string_len);
+ rte_memcpy(&dst[off], string, len % string_len);
+ } else {
+ memset(dst, 0, len);
+ }
}
return m;
@@ -1365,7 +1369,8 @@ test_ipsec_crypto_outb_burst_null_null(int i)
/* Generate input mbuf data */
for (j = 0; j < num_pkts && rc == 0; j++) {
ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
- null_plain_data, test_cfg[i].pkt_sz, 0);
+ null_plain_data, sizeof(null_plain_data),
+ test_cfg[i].pkt_sz);
if (ut_params->ibuf[j] == NULL)
rc = TEST_FAILED;
else {
@@ -1483,7 +1488,8 @@ test_ipsec_inline_crypto_inb_burst_null_null(int i)
/* Generate test mbuf data */
ut_params->obuf[j] = setup_test_string(
ts_params->mbuf_pool,
- null_plain_data, test_cfg[i].pkt_sz, 0);
+ null_plain_data, sizeof(null_plain_data),
+ test_cfg[i].pkt_sz);
if (ut_params->obuf[j] == NULL)
rc = TEST_FAILED;
}
@@ -1551,16 +1557,17 @@ test_ipsec_inline_proto_inb_burst_null_null(int i)
/* Generate inbound mbuf data */
for (j = 0; j < num_pkts && rc == 0; j++) {
- ut_params->ibuf[j] = setup_test_string(
- ts_params->mbuf_pool,
- null_plain_data, test_cfg[i].pkt_sz, 0);
+ ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
+ null_plain_data, sizeof(null_plain_data),
+ test_cfg[i].pkt_sz);
if (ut_params->ibuf[j] == NULL)
rc = TEST_FAILED;
else {
/* Generate test mbuf data */
ut_params->obuf[j] = setup_test_string(
ts_params->mbuf_pool,
- null_plain_data, test_cfg[i].pkt_sz, 0);
+ null_plain_data, sizeof(null_plain_data),
+ test_cfg[i].pkt_sz);
if (ut_params->obuf[j] == NULL)
rc = TEST_FAILED;
}
@@ -1660,7 +1667,8 @@ test_ipsec_inline_crypto_outb_burst_null_null(int i)
/* Generate test mbuf data */
for (j = 0; j < num_pkts && rc == 0; j++) {
ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
- null_plain_data, test_cfg[i].pkt_sz, 0);
+ null_plain_data, sizeof(null_plain_data),
+ test_cfg[i].pkt_sz);
if (ut_params->ibuf[0] == NULL)
rc = TEST_FAILED;
@@ -1738,15 +1746,16 @@ test_ipsec_inline_proto_outb_burst_null_null(int i)
/* Generate test mbuf data */
for (j = 0; j < num_pkts && rc == 0; j++) {
ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
- null_plain_data, test_cfg[i].pkt_sz, 0);
+ null_plain_data, sizeof(null_plain_data),
+ test_cfg[i].pkt_sz);
if (ut_params->ibuf[0] == NULL)
rc = TEST_FAILED;
if (rc == 0) {
/* Generate test tunneled mbuf data for comparison */
ut_params->obuf[j] = setup_test_string(
- ts_params->mbuf_pool,
- null_plain_data, test_cfg[i].pkt_sz, 0);
+ ts_params->mbuf_pool, null_plain_data,
+ sizeof(null_plain_data), test_cfg[i].pkt_sz);
if (ut_params->obuf[j] == NULL)
rc = TEST_FAILED;
}
@@ -1815,7 +1824,8 @@ test_ipsec_lksd_proto_inb_burst_null_null(int i)
for (j = 0; j < num_pkts && rc == 0; j++) {
/* packet with sequence number 0 is invalid */
ut_params->ibuf[j] = setup_test_string(ts_params->mbuf_pool,
- null_encrypted_data, test_cfg[i].pkt_sz, 0);
+ null_encrypted_data, sizeof(null_encrypted_data),
+ test_cfg[i].pkt_sz);
if (ut_params->ibuf[j] == NULL)
rc = TEST_FAILED;
}