| Message ID | 20220518101657.1230416-13-david.marchand@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | Fix compilation with gcc 12 | expand |
| Context | Check | Description |
|---|---|---|
| ci/intel-Testing | fail | Testing issues |
| ci/Intel-compilation | fail | Compilation issues |
| ci/iol-x86_64-compile-testing | success | Testing PASS |
| ci/iol-abi-testing | success | Testing PASS |
| ci/iol-x86_64-unit-testing | success | Testing PASS |
| ci/iol-aarch64-compile-testing | success | Testing PASS |
| ci/iol-aarch64-unit-testing | success | Testing PASS |
| ci/iol-intel-Performance | success | Performance Testing PASS |
| ci/iol-intel-Functional | success | Functional Testing PASS |
| ci/github-robot: build | success | github build: passed |
| ci/checkpatch | success | coding style OK |
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.
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) + 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; }
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(-)