[PATCH 01/12] common/cpt: fix build with GCC 12

[PATCH 01/12] common/cpt: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In function ‘fill_sg_comp_from_iov’,
    inlined from ‘cpt_kasumi_enc_prep’ at
        ../drivers/common/cpt/cpt_ucode.h:2176:8,
    inlined from ‘cpt_fc_enc_hmac_prep’ at
        ../drivers/common/cpt/cpt_ucode.h:2475:3,
    inlined from ‘fill_digest_params’ at
        ../drivers/common/cpt/cpt_ucode.h:3548:14,
    inlined from ‘otx_cpt_enq_single_sym’ at
        ../drivers/crypto/octeontx/otx_cryptodev_ops.c:541:9,
    inlined from ‘otx_cpt_enq_single_sym_sessless’ at
        ../drivers/crypto/octeontx/otx_cryptodev_ops.c:584:8,
    inlined from ‘otx_cpt_enq_single’ at
        ../drivers/crypto/octeontx/otx_cryptodev_ops.c:611:11,
    inlined from ‘otx_cpt_pkt_enqueue’ at
        ../drivers/crypto/octeontx/otx_cryptodev_ops.c:643:9,
    inlined from ‘otx_cpt_enqueue_sym’ at
        ../drivers/crypto/octeontx/otx_cryptodev_ops.c:668:9:
../drivers/common/cpt/cpt_ucode.h:415:36: error: array subscript 0 is
        outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
        [-Werror=array-bounds]
  415 |                         e_dma_addr = bufs[j].dma_addr;
      |                         ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
../drivers/common/cpt/cpt_ucode.h:416:48: error: array subscript 0 is
        outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
        [-Werror=array-bounds]
  416 |                         e_len = (size > bufs[j].size) ?
      |                                         ~~~~~~~^~~~~

For now, waive this warning until we have a proper fix.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/common/cpt/cpt_ucode.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/common/cpt/cpt_ucode.h b/drivers/common/cpt/cpt_ucode.h
index e1f2f6005d..bdf72b400c 100644
--- a/drivers/common/cpt/cpt_ucode.h
+++ b/drivers/common/cpt/cpt_ucode.h
@@ -412,9 +412,17 @@ fill_sg_comp_from_iov(sg_comp_t *list,
 				(bufs[j].size - from_offset) : size;
 			from_offset = 0;
 		} else {
+/* FIXME */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 120000)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+#endif
 			e_dma_addr = bufs[j].dma_addr;
 			e_len = (size > bufs[j].size) ?
 				bufs[j].size : size;
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 120000)
+#pragma GCC diagnostic pop
+#endif
 		}
 
 		to->u.s.len[i % 4] = rte_cpu_to_be_16(e_len);
-- 
2.36.1

Re: [PATCH 01/12] common/cpt: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:46 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> In function ‘fill_sg_comp_from_iov’,
>     inlined from ‘cpt_kasumi_enc_prep’ at
>         ../drivers/common/cpt/cpt_ucode.h:2176:8,
>     inlined from ‘cpt_fc_enc_hmac_prep’ at
>         ../drivers/common/cpt/cpt_ucode.h:2475:3,
>     inlined from ‘fill_digest_params’ at
>         ../drivers/common/cpt/cpt_ucode.h:3548:14,
>     inlined from ‘otx_cpt_enq_single_sym’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:541:9,
>     inlined from ‘otx_cpt_enq_single_sym_sessless’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:584:8,
>     inlined from ‘otx_cpt_enq_single’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:611:11,
>     inlined from ‘otx_cpt_pkt_enqueue’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:643:9,
>     inlined from ‘otx_cpt_enqueue_sym’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:668:9:
> ../drivers/common/cpt/cpt_ucode.h:415:36: error: array subscript 0 is
>         outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
>         [-Werror=array-bounds]
>   415 |                         e_dma_addr = bufs[j].dma_addr;
>       |                         ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
> ../drivers/common/cpt/cpt_ucode.h:416:48: error: array subscript 0 is
>         outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
>         [-Werror=array-bounds]
>   416 |                         e_len = (size > bufs[j].size) ?
>       |                                         ~~~~~~~^~~~~
> 
> For now, waive this warning until we have a proper fix.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
 
NAK
Please fix properly, with something like:

--- a/drivers/common/cpt/cpt_mcode_defines.h
+++ b/drivers/common/cpt/cpt_mcode_defines.h
@@ -387,7 +387,7 @@ typedef struct buf_ptr {
 /* IOV Pointer */
 typedef struct{
        int buf_cnt;
-       buf_ptr_t bufs[0];
+       buf_ptr_t bufs[];
 } iov_ptr_t;
 
 typedef struct fc_params {

Re: [PATCH 01/12] common/cpt: fix build with GCC 12

[David Marchand]

Hello maintainers,

On Wed, May 18, 2022 at 12:17 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> GCC 12 raises the following warning:
>
> In function ‘fill_sg_comp_from_iov’,
>     inlined from ‘cpt_kasumi_enc_prep’ at
>         ../drivers/common/cpt/cpt_ucode.h:2176:8,
>     inlined from ‘cpt_fc_enc_hmac_prep’ at
>         ../drivers/common/cpt/cpt_ucode.h:2475:3,
>     inlined from ‘fill_digest_params’ at
>         ../drivers/common/cpt/cpt_ucode.h:3548:14,
>     inlined from ‘otx_cpt_enq_single_sym’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:541:9,
>     inlined from ‘otx_cpt_enq_single_sym_sessless’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:584:8,
>     inlined from ‘otx_cpt_enq_single’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:611:11,
>     inlined from ‘otx_cpt_pkt_enqueue’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:643:9,
>     inlined from ‘otx_cpt_enqueue_sym’ at
>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:668:9:
> ../drivers/common/cpt/cpt_ucode.h:415:36: error: array subscript 0 is
>         outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
>         [-Werror=array-bounds]
>   415 |                         e_dma_addr = bufs[j].dma_addr;
>       |                         ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
> ../drivers/common/cpt/cpt_ucode.h:416:48: error: array subscript 0 is
>         outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
>         [-Werror=array-bounds]
>   416 |                         e_len = (size > bufs[j].size) ?
>       |                                         ~~~~~~~^~~~~
>
> For now, waive this warning until we have a proper fix.

Both common/cpt and crypto/cnxk have the same code that triggers this warning.
Can you look into this please?

Thanks.

-- 
David Marchand

RE: [EXT] Re: [PATCH 01/12] common/cpt: fix build with GCC 12

[Ankur Dwivedi]

Hi David,

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Friday, June 10, 2022 6:42 PM
>To: Anoob Joseph <anoobj@marvell.com>; Ankur Dwivedi
><adwivedi@marvell.com>
>Cc: Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
><ferruh.yigit@xilinx.com>; dpdk stable <stable@dpdk.org>; dev
><dev@dpdk.org>; Akhil Goyal <gakhil@marvell.com>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>
>Subject: [EXT] Re: [PATCH 01/12] common/cpt: fix build with GCC 12
>
>External Email
>
>----------------------------------------------------------------------
>Hello maintainers,
>
>On Wed, May 18, 2022 at 12:17 PM David Marchand
><david.marchand@redhat.com> wrote:
>>
>> GCC 12 raises the following warning:
>>
>> In function ‘fill_sg_comp_from_iov’,
>>     inlined from ‘cpt_kasumi_enc_prep’ at
>>         ../drivers/common/cpt/cpt_ucode.h:2176:8,
>>     inlined from ‘cpt_fc_enc_hmac_prep’ at
>>         ../drivers/common/cpt/cpt_ucode.h:2475:3,
>>     inlined from ‘fill_digest_params’ at
>>         ../drivers/common/cpt/cpt_ucode.h:3548:14,
>>     inlined from ‘otx_cpt_enq_single_sym’ at
>>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:541:9,
>>     inlined from ‘otx_cpt_enq_single_sym_sessless’ at
>>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:584:8,
>>     inlined from ‘otx_cpt_enq_single’ at
>>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:611:11,
>>     inlined from ‘otx_cpt_pkt_enqueue’ at
>>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:643:9,
>>     inlined from ‘otx_cpt_enqueue_sym’ at
>>         ../drivers/crypto/octeontx/otx_cryptodev_ops.c:668:9:
>> ../drivers/common/cpt/cpt_ucode.h:415:36: error: array subscript 0 is
>>         outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
>>         [-Werror=array-bounds]
>>   415 |                         e_dma_addr = bufs[j].dma_addr;
>>       |                         ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
>> ../drivers/common/cpt/cpt_ucode.h:416:48: error: array subscript 0 is
>>         outside array bounds of ‘buf_ptr_t[0]’ {aka ‘struct buf_ptr[]’}
>>         [-Werror=array-bounds]
>>   416 |                         e_len = (size > bufs[j].size) ?
>>       |                                         ~~~~~~~^~~~~
>>
>> For now, waive this warning until we have a proper fix.
>
>Both common/cpt and crypto/cnxk have the same code that triggers this
>warning.
>Can you look into this please?

We will look into the issues in common/cpt and crypto/cnxk.
>
>Thanks.
>
>--
>David Marchand

Regards,
Ankur

Re: [EXT] Re: [PATCH 01/12] common/cpt: fix build with GCC 12

[David Marchand]

On Mon, Jun 13, 2022 at 1:40 PM Ankur Dwivedi <adwivedi@marvell.com> wrote:
> >> For now, waive this warning until we have a proper fix.
> >
> >Both common/cpt and crypto/cnxk have the same code that triggers this
> >warning.
> >Can you look into this please?
>
> We will look into the issues in common/cpt and crypto/cnxk.

Any update?
Thanks.


-- 
David Marchand

RE: [EXT] Re: [PATCH 01/12] common/cpt: fix build with GCC 12

[Ankur Dwivedi]

Hi David,

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Thursday, June 16, 2022 3:00 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>
>Cc: Anoob Joseph <anoobj@marvell.com>; Thomas Monjalon
><thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@xilinx.com>; dpdk stable
><stable@dpdk.org>; dev <dev@dpdk.org>; Akhil Goyal <gakhil@marvell.com>
>Subject: Re: [EXT] Re: [PATCH 01/12] common/cpt: fix build with GCC 12
>
>On Mon, Jun 13, 2022 at 1:40 PM Ankur Dwivedi <adwivedi@marvell.com>
>wrote:
>> >> For now, waive this warning until we have a proper fix.
>> >
>> >Both common/cpt and crypto/cnxk have the same code that triggers this
>> >warning.
>> >Can you look into this please?
>>
>> We will look into the issues in common/cpt and crypto/cnxk.
>
>Any update?

We are working on the changes. Will send the patch once it is complete.

>Thanks.
>
>
>--
>David Marchand

Regards,
Ankur

[PATCH 02/12] crypto/cnxk: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In file included from ../drivers/crypto/cnxk/cn10k_cryptodev_ops.c:17:
In function ‘fill_sg_comp_from_iov’,
    inlined from ‘cpt_kasumi_enc_prep’ at
        ../drivers/crypto/cnxk/cnxk_se.h:1413:8,
    inlined from ‘cpt_fc_enc_hmac_prep’ at
        ../drivers/crypto/cnxk/cnxk_se.h:1635:9,
    inlined from ‘fill_digest_params’ at
        ../drivers/crypto/cnxk/cnxk_se.h:2524:8,
    inlined from ‘cpt_sym_inst_fill’ at
        ../drivers/crypto/cnxk/cn10k_cryptodev_ops.c:92:9,
    inlined from ‘cn10k_cpt_fill_inst.constprop.isra’ at
        ../drivers/crypto/cnxk/cn10k_cryptodev_ops.c:146:10:
../drivers/crypto/cnxk/cnxk_se.h:208:52: error: array subscript 0 is
        outside array bounds of ‘struct roc_se_buf_ptr[0]’
        [-Werror=array-bounds]
  208 |             e_vaddr = (uint64_t)bufs[j].vaddr;
      |                                 ~~~~~~~^~~~~~
../drivers/crypto/cnxk/cnxk_se.h:209:48: error: array subscript 0 is
        outside array bounds of ‘struct roc_se_buf_ptr[0]’
        [-Werror=array-bounds]
  209 |             e_len = (size > bufs[j].size) ? bufs[j].size : size;
      |                             ~~~~~~~^~~~~

For now, waive this warning until we have a proper fix.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/crypto/cnxk/cnxk_se.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/crypto/cnxk/cnxk_se.h b/drivers/crypto/cnxk/cnxk_se.h
index ce7ca2eda9..c9d147601f 100644
--- a/drivers/crypto/cnxk/cnxk_se.h
+++ b/drivers/crypto/cnxk/cnxk_se.h
@@ -205,8 +205,16 @@ fill_sg_comp_from_iov(struct roc_se_sglist_comp *list, uint32_t i,
 					size;
 			from_offset = 0;
 		} else {
+/* FIXME */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 120000)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+#endif
 			e_vaddr = (uint64_t)bufs[j].vaddr;
 			e_len = (size > bufs[j].size) ? bufs[j].size : size;
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 120000)
+#pragma GCC diagnostic pop
+#endif
 		}
 
 		to->u.s.len[i % 4] = rte_cpu_to_be_16(e_len);
-- 
2.36.1

Re: [PATCH 02/12] crypto/cnxk: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:47 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> In file included from ../drivers/crypto/cnxk/cn10k_cryptodev_ops.c:17:
> In function ‘fill_sg_comp_from_iov’,
>     inlined from ‘cpt_kasumi_enc_prep’ at
>         ../drivers/crypto/cnxk/cnxk_se.h:1413:8,
>     inlined from ‘cpt_fc_enc_hmac_prep’ at
>         ../drivers/crypto/cnxk/cnxk_se.h:1635:9,
>     inlined from ‘fill_digest_params’ at
>         ../drivers/crypto/cnxk/cnxk_se.h:2524:8,
>     inlined from ‘cpt_sym_inst_fill’ at
>         ../drivers/crypto/cnxk/cn10k_cryptodev_ops.c:92:9,
>     inlined from ‘cn10k_cpt_fill_inst.constprop.isra’ at
>         ../drivers/crypto/cnxk/cn10k_cryptodev_ops.c:146:10:
> ../drivers/crypto/cnxk/cnxk_se.h:208:52: error: array subscript 0 is
>         outside array bounds of ‘struct roc_se_buf_ptr[0]’
>         [-Werror=array-bounds]
>   208 |             e_vaddr = (uint64_t)bufs[j].vaddr;
>       |                                 ~~~~~~~^~~~~~
> ../drivers/crypto/cnxk/cnxk_se.h:209:48: error: array subscript 0 is
>         outside array bounds of ‘struct roc_se_buf_ptr[0]’
>         [-Werror=array-bounds]
>   209 |             e_len = (size > bufs[j].size) ? bufs[j].size : size;
>       |                             ~~~~~~~^~~~~
> 
> For now, waive this warning until we have a proper fix.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

If you fix iov_ptr_t to be flexible array this won't be needed.

[PATCH 03/12] crypto/ipsec_mb: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In function ‘__rte_ring_enqueue_elems_64’,
    inlined from ‘__rte_ring_enqueue_elems’ at
        ../lib/ring/rte_ring_elem_pvt.h:130:3,
    inlined from ‘__rte_ring_do_hts_enqueue_elem’ at
        ../lib/ring/rte_ring_hts_elem_pvt.h:196:3,
    inlined from ‘rte_ring_mp_hts_enqueue_burst_elem’ at
        ../lib/ring/rte_ring_hts.h:110:9,
    inlined from ‘rte_ring_enqueue_burst_elem’ at
        ../lib/ring/rte_ring_elem.h:577:10,
    inlined from ‘rte_ring_enqueue_burst’ at
        ../lib/ring/rte_ring.h:738:9,
    inlined from ‘process_op_bit’ at
        ../drivers/crypto/ipsec_mb/pmd_snow3g.c:425:16,
    inlined from ‘snow3g_pmd_dequeue_burst’ at
        ../drivers/crypto/ipsec_mb/pmd_snow3g.c:484:20:
../lib/ring/rte_ring_elem_pvt.h:68:44: error: array subscript 1 is
        outside array bounds of ‘struct rte_crypto_op[0]’
        [-Werror=array-bounds]
   68 |                         ring[idx + 1] = obj[i + 1];
      |                                         ~~~^~~~~~~
../drivers/crypto/ipsec_mb/pmd_snow3g.c: In function
        ‘snow3g_pmd_dequeue_burst’:
../drivers/crypto/ipsec_mb/pmd_snow3g.c:434:1: note:
        at offset 8 into object ‘op’ of size 8
  434 | snow3g_pmd_dequeue_burst(void *queue_pair,
      | ^~~~~~~~~~~~~~~~~~~~~~~~

Validate that one (exactly) op has been processed or return early.

Fixes: b537abdbee74 ("crypto/snow3g: support bit-level operations")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/crypto/ipsec_mb/pmd_snow3g.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/ipsec_mb/pmd_snow3g.c b/drivers/crypto/ipsec_mb/pmd_snow3g.c
index ebc9a0b562..9a85f46721 100644
--- a/drivers/crypto/ipsec_mb/pmd_snow3g.c
+++ b/drivers/crypto/ipsec_mb/pmd_snow3g.c
@@ -422,12 +422,13 @@ process_op_bit(struct rte_crypto_op *op, struct snow3g_session *session,
 		op->sym->session = NULL;
 	}
 
-	enqueued_op = rte_ring_enqueue_burst(qp->ingress_queue,
-			(void **)&op, processed_op, NULL);
+	if (unlikely(processed_op != 1))
+		return 0;
+	enqueued_op = rte_ring_enqueue(qp->ingress_queue, op);
 	qp->stats.enqueued_count += enqueued_op;
 	*accumulated_enqueued_ops += enqueued_op;
 
-	return enqueued_op;
+	return 1;
 }
 
 static uint16_t
-- 
2.36.1

Re: [PATCH 03/12] crypto/ipsec_mb: fix build with GCC 12

[Bruce Richardson]

On Wed, May 18, 2022 at 12:16:48PM +0200, David Marchand wrote:
> GCC 12 raises the following warning:
> 
> In function ‘__rte_ring_enqueue_elems_64’,
>     inlined from ‘__rte_ring_enqueue_elems’ at
>         ../lib/ring/rte_ring_elem_pvt.h:130:3,
>     inlined from ‘__rte_ring_do_hts_enqueue_elem’ at
>         ../lib/ring/rte_ring_hts_elem_pvt.h:196:3,
>     inlined from ‘rte_ring_mp_hts_enqueue_burst_elem’ at
>         ../lib/ring/rte_ring_hts.h:110:9,
>     inlined from ‘rte_ring_enqueue_burst_elem’ at
>         ../lib/ring/rte_ring_elem.h:577:10,
>     inlined from ‘rte_ring_enqueue_burst’ at
>         ../lib/ring/rte_ring.h:738:9,
>     inlined from ‘process_op_bit’ at
>         ../drivers/crypto/ipsec_mb/pmd_snow3g.c:425:16,
>     inlined from ‘snow3g_pmd_dequeue_burst’ at
>         ../drivers/crypto/ipsec_mb/pmd_snow3g.c:484:20:
> ../lib/ring/rte_ring_elem_pvt.h:68:44: error: array subscript 1 is
>         outside array bounds of ‘struct rte_crypto_op[0]’
>         [-Werror=array-bounds]
>    68 |                         ring[idx + 1] = obj[i + 1];
>       |                                         ~~~^~~~~~~
> ../drivers/crypto/ipsec_mb/pmd_snow3g.c: In function
>         ‘snow3g_pmd_dequeue_burst’:
> ../drivers/crypto/ipsec_mb/pmd_snow3g.c:434:1: note:
>         at offset 8 into object ‘op’ of size 8
>   434 | snow3g_pmd_dequeue_burst(void *queue_pair,
>       | ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> Validate that one (exactly) op has been processed or return early.
> 
> Fixes: b537abdbee74 ("crypto/snow3g: support bit-level operations")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/crypto/ipsec_mb/pmd_snow3g.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/ipsec_mb/pmd_snow3g.c b/drivers/crypto/ipsec_mb/pmd_snow3g.c
> index ebc9a0b562..9a85f46721 100644
> --- a/drivers/crypto/ipsec_mb/pmd_snow3g.c
> +++ b/drivers/crypto/ipsec_mb/pmd_snow3g.c
> @@ -422,12 +422,13 @@ process_op_bit(struct rte_crypto_op *op, struct snow3g_session *session,
>  		op->sym->session = NULL;
>  	}
>  
> -	enqueued_op = rte_ring_enqueue_burst(qp->ingress_queue,
> -			(void **)&op, processed_op, NULL);
> +	if (unlikely(processed_op != 1))
> +		return 0;
> +	enqueued_op = rte_ring_enqueue(qp->ingress_queue, op);

As a fix for the compiler warning this looks ok, but question for
maintainer would be - should this check for processed_op != 1 not go
earlier in the function, immediately after the switch statement?

Fan, Pablo, can you please comment?

/Bruce

Re: [PATCH 03/12] crypto/ipsec_mb: fix build with GCC 12

[David Marchand]

On Thu, Jun 2, 2022 at 11:50 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, May 18, 2022 at 12:16:48PM +0200, David Marchand wrote:
> > GCC 12 raises the following warning:
> >
> > In function ‘__rte_ring_enqueue_elems_64’,
> >     inlined from ‘__rte_ring_enqueue_elems’ at
> >         ../lib/ring/rte_ring_elem_pvt.h:130:3,
> >     inlined from ‘__rte_ring_do_hts_enqueue_elem’ at
> >         ../lib/ring/rte_ring_hts_elem_pvt.h:196:3,
> >     inlined from ‘rte_ring_mp_hts_enqueue_burst_elem’ at
> >         ../lib/ring/rte_ring_hts.h:110:9,
> >     inlined from ‘rte_ring_enqueue_burst_elem’ at
> >         ../lib/ring/rte_ring_elem.h:577:10,
> >     inlined from ‘rte_ring_enqueue_burst’ at
> >         ../lib/ring/rte_ring.h:738:9,
> >     inlined from ‘process_op_bit’ at
> >         ../drivers/crypto/ipsec_mb/pmd_snow3g.c:425:16,
> >     inlined from ‘snow3g_pmd_dequeue_burst’ at
> >         ../drivers/crypto/ipsec_mb/pmd_snow3g.c:484:20:
> > ../lib/ring/rte_ring_elem_pvt.h:68:44: error: array subscript 1 is
> >         outside array bounds of ‘struct rte_crypto_op[0]’
> >         [-Werror=array-bounds]
> >    68 |                         ring[idx + 1] = obj[i + 1];
> >       |                                         ~~~^~~~~~~
> > ../drivers/crypto/ipsec_mb/pmd_snow3g.c: In function
> >         ‘snow3g_pmd_dequeue_burst’:
> > ../drivers/crypto/ipsec_mb/pmd_snow3g.c:434:1: note:
> >         at offset 8 into object ‘op’ of size 8
> >   434 | snow3g_pmd_dequeue_burst(void *queue_pair,
> >       | ^~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Validate that one (exactly) op has been processed or return early.
> >
> > Fixes: b537abdbee74 ("crypto/snow3g: support bit-level operations")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  drivers/crypto/ipsec_mb/pmd_snow3g.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/crypto/ipsec_mb/pmd_snow3g.c b/drivers/crypto/ipsec_mb/pmd_snow3g.c
> > index ebc9a0b562..9a85f46721 100644
> > --- a/drivers/crypto/ipsec_mb/pmd_snow3g.c
> > +++ b/drivers/crypto/ipsec_mb/pmd_snow3g.c
> > @@ -422,12 +422,13 @@ process_op_bit(struct rte_crypto_op *op, struct snow3g_session *session,
> >               op->sym->session = NULL;
> >       }
> >
> > -     enqueued_op = rte_ring_enqueue_burst(qp->ingress_queue,
> > -                     (void **)&op, processed_op, NULL);
> > +     if (unlikely(processed_op != 1))
> > +             return 0;
> > +     enqueued_op = rte_ring_enqueue(qp->ingress_queue, op);
>
> As a fix for the compiler warning this looks ok, but question for
> maintainer would be - should this check for processed_op != 1 not go
> earlier in the function, immediately after the switch statement?
>
> Fan, Pablo, can you please comment?

Fan? Pablo?


-- 
David Marchand

Re: [PATCH 03/12] crypto/ipsec_mb: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:48 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> In function ‘__rte_ring_enqueue_elems_64’,
>     inlined from ‘__rte_ring_enqueue_elems’ at
>         ../lib/ring/rte_ring_elem_pvt.h:130:3,
>     inlined from ‘__rte_ring_do_hts_enqueue_elem’ at
>         ../lib/ring/rte_ring_hts_elem_pvt.h:196:3,
>     inlined from ‘rte_ring_mp_hts_enqueue_burst_elem’ at
>         ../lib/ring/rte_ring_hts.h:110:9,
>     inlined from ‘rte_ring_enqueue_burst_elem’ at
>         ../lib/ring/rte_ring_elem.h:577:10,
>     inlined from ‘rte_ring_enqueue_burst’ at
>         ../lib/ring/rte_ring.h:738:9,
>     inlined from ‘process_op_bit’ at
>         ../drivers/crypto/ipsec_mb/pmd_snow3g.c:425:16,
>     inlined from ‘snow3g_pmd_dequeue_burst’ at
>         ../drivers/crypto/ipsec_mb/pmd_snow3g.c:484:20:
> ../lib/ring/rte_ring_elem_pvt.h:68:44: error: array subscript 1 is
>         outside array bounds of ‘struct rte_crypto_op[0]’
>         [-Werror=array-bounds]
>    68 |                         ring[idx + 1] = obj[i + 1];
>       |                                         ~~~^~~~~~~
> ../drivers/crypto/ipsec_mb/pmd_snow3g.c: In function
>         ‘snow3g_pmd_dequeue_burst’:
> ../drivers/crypto/ipsec_mb/pmd_snow3g.c:434:1: note:
>         at offset 8 into object ‘op’ of size 8
>   434 | snow3g_pmd_dequeue_burst(void *queue_pair,
>       | ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> Validate that one (exactly) op has been processed or return early.
> 
> Fixes: b537abdbee74 ("crypto/snow3g: support bit-level operations")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 04/12] net/ena: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In file included from ../lib/mempool/rte_mempool.h:46,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/net/rte_ether.h:22,
                 from ../drivers/net/ena/ena_ethdev.h:10,
                 from ../drivers/net/ena/ena_rss.c:6:
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../lib/eal/x86/include/rte_memcpy.h:370:9: warning: array subscript 64 is
        outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’}
        [-Warray-bounds]
  370 | rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/ena/ena_rss.c:51:24: note: while referencing ‘default_key’
   51 | static uint8_t default_key[ENA_HASH_KEY_SIZE];
      |                ^~~~~~~~~~~

This is a false positive because the copied size is checked against
ENA_HASH_KEY_SIZE in a (build) assert.
Silence this warning by calling memcpy with the minimal size.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/ena/ena_rss.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ena/ena_rss.c b/drivers/net/ena/ena_rss.c
index b6c4f76e38..b682d01c20 100644
--- a/drivers/net/ena/ena_rss.c
+++ b/drivers/net/ena/ena_rss.c
@@ -51,15 +51,14 @@ void ena_rss_key_fill(void *key, size_t size)
 	static uint8_t default_key[ENA_HASH_KEY_SIZE];
 	size_t i;
 
-	RTE_ASSERT(size <= ENA_HASH_KEY_SIZE);
-
 	if (!key_generated) {
-		for (i = 0; i < ENA_HASH_KEY_SIZE; ++i)
+		for (i = 0; i < RTE_DIM(default_key); ++i)
 			default_key[i] = rte_rand() & 0xff;
 		key_generated = true;
 	}
 
-	rte_memcpy(key, default_key, size);
+	RTE_ASSERT(size <= sizeof(default_key));
+	rte_memcpy(key, default_key, RTE_MIN(size, sizeof(default_key)));
 }
 
 int ena_rss_reta_update(struct rte_eth_dev *dev,
-- 
2.36.1

Re: [PATCH 04/12] net/ena: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:49 +0200
David Marchand <david.marchand@redhat.com> wrote:

> +		for (i = 0; i < RTE_DIM(default_key); ++i)
>  			default_key[i] = rte_rand() & 0xff;

We should have rte_random_bytes() functionality if this gets
used often.

Also, worth considering dropping DPDK random number generator
in userspace for security reasons and just using more secure kernel code.

RE: [PATCH 04/12] net/ena: fix build with GCC 12

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, 20 May 2022 22.28
> 
> On Wed, 18 May 2022 12:16:49 +0200
> David Marchand <david.marchand@redhat.com> wrote:
> 
> > +		for (i = 0; i < RTE_DIM(default_key); ++i)
> >  			default_key[i] = rte_rand() & 0xff;
> 
> We should have rte_random_bytes() functionality if this gets
> used often.

Since the other pseudorandom functions are called rand, such a function should be named rte_rand_bytes().

> 
> Also, worth considering dropping DPDK random number generator
> in userspace for security reasons and just using more secure kernel
> code.

Absolutely not! We need a fast pseudorandom number generator in DPDK.

If anything, we could consider renaming the functions and header file to reflect that they are pseudorandom number generators, and not (cryptographically) random generators. That would cause an API/ABI breakage, so it's probably not going to happen. ;-)

Re: [PATCH 04/12] net/ena: fix build with GCC 12

[Stephen Hemminger]

On Sat, 21 May 2022 11:49:47 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > 
> > Also, worth considering dropping DPDK random number generator
> > in userspace for security reasons and just using more secure kernel
> > code.  
> 
> Absolutely not! We need a fast pseudorandom number generator in DPDK.
> 
> If anything, we could consider renaming the functions and header file to reflect that they are pseudorandom number generators, and not (cryptographically) random generators. That would cause an API/ABI breakage, so it's probably not going to happen. ;-)


The Linux kernel has received an way more attention on random numbers than
DPDK. If you follow the history, what happens is that a simple dumb LCG
or similar random number generator gets invented, and then gets used for
lots of things that people don't think need a strong generator.

Followed by DoS and other attacks where the weak random number generator
is broken when used for doing things like creating sequence numbers of
TCP port assignment.  This is then followed by even more work on the
kernel random number generator to make the default random number generator
stronger.

I bring up this history, so that DPDK won't have to repeat it.

Right now the DPDK random number generator is insecure because it uses
long but weak PRNG and never reseeds itself.

See:
https://lwn.net/Articles/884875/

There is also FIPS to consider.
https://lwn.net/Articles/877607/

Since random number generators are hard, prefer that someone else do it :-)

RE: [PATCH 04/12] net/ena: fix build with GCC 12

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 21 May 2022 18.24
> 
> On Sat, 21 May 2022 11:49:47 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > >
> > > Also, worth considering dropping DPDK random number generator
> > > in userspace for security reasons and just using more secure kernel
> > > code.
> >
> > Absolutely not! We need a fast pseudorandom number generator in DPDK.
> >
> > If anything, we could consider renaming the functions and header file
> to reflect that they are pseudorandom number generators, and not
> (cryptographically) random generators. That would cause an API/ABI
> breakage, so it's probably not going to happen. ;-)
> 
> 
> The Linux kernel has received an way more attention on random numbers
> than
> DPDK. If you follow the history, what happens is that a simple dumb LCG
> or similar random number generator gets invented, and then gets used
> for
> lots of things that people don't think need a strong generator.
> 
> Followed by DoS and other attacks where the weak random number
> generator
> is broken when used for doing things like creating sequence numbers of
> TCP port assignment.  This is then followed by even more work on the
> kernel random number generator to make the default random number
> generator
> stronger.
> 
> I bring up this history, so that DPDK won't have to repeat it.
> 
> Right now the DPDK random number generator is insecure because it uses
> long but weak PRNG and never reseeds itself.
> 
> See:
> https://lwn.net/Articles/884875/
> 
> There is also FIPS to consider.
> https://lwn.net/Articles/877607/
> 
> Since random number generators are hard, prefer that someone else do it
> :-)

First of all, I would like to thank you for the history lesson and references, Stephen, it made my Saturday evening much more nerdy and interesting than expected! Not being a native English speaker, please understand that I mean this sincerely. I really enjoyed reading about this corner of the Linux kernel history.

Overall, I think that RNGs generally fall into two categories: Unsafe (regardless how advanced) and safe for crypto use.

It should be OK for DPDK to provide something blazing fast, but unsafe. The DPDK documentation clearly states that the provided random functions are not safe for crypto, so I would expect the developers to use them accordingly.

Having thought about it, I came to this conclusion: Regardless if we provide unsafe RNG functions in DPDK or not, it is ultimately up to the application developers to choose which RNG category to use for different purposes. If we don't provide something fast, developers will just use the standard rand48() functions or similar. And a blazing fast (but unsafe) RNG is useful for simple things like pseudo-random packet sampling in the data plane.

Who would have thought that using a simple RNG for TCP port assignment could end up being a security problem... The developers will always have a choice between secure and fast, and the risk of a developer making the wrong decision is not affected by DPDK providing some unsafe RNG or not.

At a higher level, I come to think of the RFCs, which all have a Security Considerations chapter. Ideally, all patches had such a chapter, and all reviews considered the security aspects, so someone would catch the use of an unsafe RNG where a safe RNG should be used. Removing the rand() functions from DPDK will not have the desired effect, only raising security awareness will.

And just to leave off where you left off: I 100 % agree that we should not try to invent our own crypto safe RNG!

PS: I assume that safe RNGs cannot generate numbers at the same rate as unsafe RNGs. If this was not generally true, there should be no need to use unsafe RNGs (except for test purposes, where reproducibility is a requirement).

In cases where the safe RNG can generate numbers at a sufficiently high rate, why not use it? This, however, requires that the application developer knows both the required rate and the rate of the safe RNG, which I guess very few developers do.

Re: [PATCH 04/12] net/ena: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:49 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> In file included from ../lib/mempool/rte_mempool.h:46,
>                  from ../lib/mbuf/rte_mbuf.h:38,
>                  from ../lib/net/rte_ether.h:22,
>                  from ../drivers/net/ena/ena_ethdev.h:10,
>                  from ../drivers/net/ena/ena_rss.c:6:
> ../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
> ../lib/eal/x86/include/rte_memcpy.h:370:9: warning: array subscript 64 is
>         outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’}
>         [-Warray-bounds]
>   370 | rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/net/ena/ena_rss.c:51:24: note: while referencing ‘default_key’
>    51 | static uint8_t default_key[ENA_HASH_KEY_SIZE];
>       |                ^~~~~~~~~~~
> 
> This is a false positive because the copied size is checked against
> ENA_HASH_KEY_SIZE in a (build) assert.
> Silence this warning by calling memcpy with the minimal size.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 05/12] net/enetfec: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

../drivers/net/enetfec/enet_ethdev.c: In function
        ‘enetfec_rx_queue_setup’:
../drivers/net/enetfec/enet_ethdev.c:473:9: error: array
        subscript 1 is
    above array bounds of ‘uint32_t[1]’ {aka ‘unsigned int[1]’}
        [-Werror=array-bounds]
  473 | rte_write32(rte_cpu_to_le_32(fep->bd_addr_p_r[queue_idx]),
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  474 |     (uint8_t *)fep->hw_baseaddr_v + ENETFEC_RD_START(queue_idx));
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../drivers/net/enetfec/enet_ethdev.c:9:
../drivers/net/enetfec/enet_ethdev.h:113:33: note: while referencing
        ‘bd_addr_p_r’
  113 | uint32_t                bd_addr_p_r[ENETFEC_MAX_Q];
      |                                 ^~~~~~~~~~~

This driver properly announces that it only supports 1 rxq.
Silence this warning by adding an explicit check on the queue id.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/enetfec/enet_ethdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
index 714f8ac7ec..c938e58204 100644
--- a/drivers/net/enetfec/enet_ethdev.c
+++ b/drivers/net/enetfec/enet_ethdev.c
@@ -2,9 +2,12 @@
  * Copyright 2020-2021 NXP
  */
 
+#include <inttypes.h>
+
 #include <ethdev_vdev.h>
 #include <ethdev_driver.h>
 #include <rte_io.h>
+
 #include "enet_pmd_logs.h"
 #include "enet_ethdev.h"
 #include "enet_regs.h"
@@ -454,6 +457,12 @@ enetfec_rx_queue_setup(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
+	if (queue_idx >= ENETFEC_MAX_Q) {
+		ENETFEC_PMD_ERR("Invalid queue id %" PRIu16 ", max %d\n",
+			queue_idx, ENETFEC_MAX_Q);
+		return -EINVAL;
+	}
+
 	/* allocate receive queue */
 	rxq = rte_zmalloc(NULL, sizeof(*rxq), RTE_CACHE_LINE_SIZE);
 	if (rxq == NULL) {
-- 
2.36.1

Re: [PATCH 05/12] net/enetfec: fix build with GCC 12

[David Marchand]

On Wed, May 18, 2022 at 12:17 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> GCC 12 raises the following warning:
>
> ../drivers/net/enetfec/enet_ethdev.c: In function
>         ‘enetfec_rx_queue_setup’:
> ../drivers/net/enetfec/enet_ethdev.c:473:9: error: array
>         subscript 1 is
>     above array bounds of ‘uint32_t[1]’ {aka ‘unsigned int[1]’}
>         [-Werror=array-bounds]
>   473 | rte_write32(rte_cpu_to_le_32(fep->bd_addr_p_r[queue_idx]),
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   474 |     (uint8_t *)fep->hw_baseaddr_v + ENETFEC_RD_START(queue_idx));
>       |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../drivers/net/enetfec/enet_ethdev.c:9:
> ../drivers/net/enetfec/enet_ethdev.h:113:33: note: while referencing
>         ‘bd_addr_p_r’
>   113 | uint32_t                bd_addr_p_r[ENETFEC_MAX_Q];
>       |                                 ^~~~~~~~~~~
>
> This driver properly announces that it only supports 1 rxq.
> Silence this warning by adding an explicit check on the queue id.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Any comment from driver maintainers?
Thanks.


-- 
David Marchand

Re: [PATCH 05/12] net/enetfec: fix build with GCC 12

[Sachin Saxena (OSS)]

Hello David,

I understood and agree with your suggestion. We are using GCC 11.3 where 
we were not seeing this warning.
We will fix this on priority and submit the patch asap.

regards,
Sachin Saxena

On 6/10/2022 6:38 PM, David Marchand wrote:
> On Wed, May 18, 2022 at 12:17 PM David Marchand
> <david.marchand@redhat.com> wrote:
>> GCC 12 raises the following warning:
>>
>> ../drivers/net/enetfec/enet_ethdev.c: In function
>>          ‘enetfec_rx_queue_setup’:
>> ../drivers/net/enetfec/enet_ethdev.c:473:9: error: array
>>          subscript 1 is
>>      above array bounds of ‘uint32_t[1]’ {aka ‘unsigned int[1]’}
>>          [-Werror=array-bounds]
>>    473 | rte_write32(rte_cpu_to_le_32(fep->bd_addr_p_r[queue_idx]),
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>    474 |     (uint8_t *)fep->hw_baseaddr_v + ENETFEC_RD_START(queue_idx));
>>        |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from ../drivers/net/enetfec/enet_ethdev.c:9:
>> ../drivers/net/enetfec/enet_ethdev.h:113:33: note: while referencing
>>          ‘bd_addr_p_r’
>>    113 | uint32_t                bd_addr_p_r[ENETFEC_MAX_Q];
>>        |                                 ^~~~~~~~~~~
>>
>> This driver properly announces that it only supports 1 rxq.
>> Silence this warning by adding an explicit check on the queue id.
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Any comment from driver maintainers?
> Thanks.
>
>

Re: [PATCH 05/12] net/enetfec: fix build with GCC 12

[Sachin Saxena (OSS)]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

Acked-by: Sachin Saxena <sachin.saxena@nxp.com>

On 6/10/2022 6:38 PM, David Marchand wrote:
> On Wed, May 18, 2022 at 12:17 PM David Marchand
> <david.marchand@redhat.com> wrote:
>> GCC 12 raises the following warning:
>>
>> ../drivers/net/enetfec/enet_ethdev.c: In function
>>          ‘enetfec_rx_queue_setup’:
>> ../drivers/net/enetfec/enet_ethdev.c:473:9: error: array
>>          subscript 1 is
>>      above array bounds of ‘uint32_t[1]’ {aka ‘unsigned int[1]’}
>>          [-Werror=array-bounds]
>>    473 | rte_write32(rte_cpu_to_le_32(fep->bd_addr_p_r[queue_idx]),
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>    474 |     (uint8_t *)fep->hw_baseaddr_v + ENETFEC_RD_START(queue_idx));
>>        |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from ../drivers/net/enetfec/enet_ethdev.c:9:
>> ../drivers/net/enetfec/enet_ethdev.h:113:33: note: while referencing
>>          ‘bd_addr_p_r’
>>    113 | uint32_t                bd_addr_p_r[ENETFEC_MAX_Q];
>>        |                                 ^~~~~~~~~~~
>>
>> This driver properly announces that it only supports 1 rxq.
>> Silence this warning by adding an explicit check on the queue id.
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Any comment from driver maintainers?
> Thanks.
>
>


[-- Attachment #2: Type: text/html, Size: 2127 bytes --]

Re: [PATCH 05/12] net/enetfec: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:50 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> ../drivers/net/enetfec/enet_ethdev.c: In function
>         ‘enetfec_rx_queue_setup’:
> ../drivers/net/enetfec/enet_ethdev.c:473:9: error: array
>         subscript 1 is
>     above array bounds of ‘uint32_t[1]’ {aka ‘unsigned int[1]’}
>         [-Werror=array-bounds]
>   473 | rte_write32(rte_cpu_to_le_32(fep->bd_addr_p_r[queue_idx]),
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   474 |     (uint8_t *)fep->hw_baseaddr_v + ENETFEC_RD_START(queue_idx));
>       |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ../drivers/net/enetfec/enet_ethdev.c:9:
> ../drivers/net/enetfec/enet_ethdev.h:113:33: note: while referencing
>         ‘bd_addr_p_r’
>   113 | uint32_t                bd_addr_p_r[ENETFEC_MAX_Q];
>       |                                 ^~~~~~~~~~~
> 
> This driver properly announces that it only supports 1 rxq.
> Silence this warning by adding an explicit check on the queue id.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 06/12] net/ice: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In file included from ../lib/mempool/rte_mempool.h:46,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/net/rte_ether.h:22,
                 from ../lib/ethdev/rte_ethdev.h:172,
                 from ../lib/ethdev/ethdev_driver.h:22,
                 from ../lib/ethdev/ethdev_pci.h:17,
                 from ../drivers/net/ice/ice_ethdev.c:6:
../drivers/net/ice/ice_ethdev.c: In function ‘ice_dev_configure’:
../lib/eal/x86/include/rte_memcpy.h:370:9: warning: array subscript 64 is
        outside array bounds of ‘struct ice_aqc_get_set_rss_keys[1]’
        [-Warray-bounds]
  370 | rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/ice/ice_ethdev.c:3202:41: note: while referencing ‘key’
 3202 |         struct ice_aqc_get_set_rss_keys key;
      |                                         ^~~

Restrict copy to minimum size.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/ice/ice_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 00ac2bb191..d69d480268 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -3263,7 +3263,8 @@ static int ice_init_rss(struct ice_pf *pf)
 			   RTE_MIN(rss_conf->rss_key_len,
 				   vsi->rss_key_size));
 
-	rte_memcpy(key.standard_rss_key, vsi->rss_key, vsi->rss_key_size);
+	rte_memcpy(key.standard_rss_key, vsi->rss_key,
+		RTE_MIN(sizeof(key.standard_rss_key), vsi->rss_key_size));
 	ret = ice_aq_set_rss_key(hw, vsi->idx, &key);
 	if (ret)
 		goto out;
-- 
2.36.1

Re: [PATCH 06/12] net/ice: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:51 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> In file included from ../lib/mempool/rte_mempool.h:46,
>                  from ../lib/mbuf/rte_mbuf.h:38,
>                  from ../lib/net/rte_ether.h:22,
>                  from ../lib/ethdev/rte_ethdev.h:172,
>                  from ../lib/ethdev/ethdev_driver.h:22,
>                  from ../lib/ethdev/ethdev_pci.h:17,
>                  from ../drivers/net/ice/ice_ethdev.c:6:
> ../drivers/net/ice/ice_ethdev.c: In function ‘ice_dev_configure’:
> ../lib/eal/x86/include/rte_memcpy.h:370:9: warning: array subscript 64 is
>         outside array bounds of ‘struct ice_aqc_get_set_rss_keys[1]’
>         [-Warray-bounds]
>   370 | rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/net/ice/ice_ethdev.c:3202:41: note: while referencing ‘key’
>  3202 |         struct ice_aqc_get_set_rss_keys key;
>       |                                         ^~~
> 
> Restrict copy to minimum size.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 07/12] net/ice/base: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

../drivers/net/ice/base/ice_switch.c: In function ‘ice_add_sw_recipe’:
../drivers/net/ice/base/ice_switch.c:7219:61: error: writing 1 byte
        into a region of size 0 [-Werror=stringop-overflow=]
 7219 |         buf[recps].content.lkup_indx[i + 1] = entry->fv_idx[i];
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
In file included from ../drivers/net/ice/base/ice_controlq.h:8,
                 from ../drivers/net/ice/base/ice_type.h:54,
                 from ../drivers/net/ice/base/ice_common.h:8,
                 from ../drivers/net/ice/base/ice_switch.h:8,
                 from ../drivers/net/ice/base/ice_switch.c:5:
../drivers/net/ice/base/ice_adminq_cmd.h:744:12: note: at offset 5
        into destination object ‘lkup_indx’ of size 5
  744 |         u8 lkup_indx[5];
      |            ^~~~~~~~~

Since this code is in the base driver, waive the check until the base
driver is fixed by the relevant people.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/ice/base/meson.build | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ice/base/meson.build b/drivers/net/ice/base/meson.build
index 3cf4ce05fa..89d8c5eba1 100644
--- a/drivers/net/ice/base/meson.build
+++ b/drivers/net/ice/base/meson.build
@@ -40,6 +40,11 @@ if (toolchain == 'gcc' and cc.version().version_compare('>=11.0.0'))
     error_cflags += ['-Wno-array-bounds']
 endif
 
+# FIXME
+if (toolchain == 'gcc' and cc.version().version_compare('>=12.0.0'))
+    error_cflags += ['-Wno-stringop-overflow']
+endif
+
 if is_windows and cc.get_id() != 'clang'
     cflags += ['-fno-asynchronous-unwind-tables']
 endif
-- 
2.36.1

[PATCH 08/12] net/qede/base: fix build with GCC 12

[David Marchand]

GCC raises the following warning:

In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at
        ../lib/eal/x86/include/rte_memcpy.h:320:2,
    inlined from ‘rte_mov128’ at
        ../lib/eal/x86/include/rte_memcpy.h:342: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 ‘__ecore_mcp_cmd_and_union’ at
        ../drivers/net/qede/base/ecore_mcp.c:541:3,
    inlined from ‘_ecore_mcp_cmd_and_union’ at
        ../drivers/net/qede/base/ecore_mcp.c:638:2,
    inlined from ‘ecore_mcp_cmd_and_union’ at
        ../drivers/net/qede/base/ecore_mcp.c:742:9:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
        array subscript 1 is outside array bounds of
        ‘union drv_union_data[1]’ [-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../drivers/net/qede/base/ecore_mcp.c: In function
        ‘ecore_mcp_cmd_and_union’:
../drivers/net/qede/base/ecore_mcp.c:533:30: note: at offset 32 into
        object ‘union_data’ of size 32
  533 |         union drv_union_data union_data;
      |                              ^~~~~~~~~~

Since this code is in the base driver, waive the check until the base
driver is fixed by the relevant people.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/qede/base/meson.build | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/qede/base/meson.build b/drivers/net/qede/base/meson.build
index 4ad177b478..c7b19be20a 100644
--- a/drivers/net/qede/base/meson.build
+++ b/drivers/net/qede/base/meson.build
@@ -44,6 +44,11 @@ error_cflags = [
         '-Wno-sometimes-uninitialized',
         '-Wno-pointer-bool-conversion',
 ]
+# FIXME
+if (toolchain == 'gcc' and cc.version().version_compare('>=12.0.0'))
+    error_cflags += ['-Wno-array-bounds']
+endif
+
 c_args = cflags
 foreach flag: error_cflags
         if cc.has_argument(flag)
-- 
2.36.1

Re: [PATCH 08/12] net/qede/base: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:53 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC raises the following warning:
> 
> In function ‘_mm256_storeu_si256’,
>     inlined from ‘rte_mov32’ at
>         ../lib/eal/x86/include/rte_memcpy.h:320:2,
>     inlined from ‘rte_mov128’ at
>         ../lib/eal/x86/include/rte_memcpy.h:342: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 ‘__ecore_mcp_cmd_and_union’ at
>         ../drivers/net/qede/base/ecore_mcp.c:541:3,
>     inlined from ‘_ecore_mcp_cmd_and_union’ at
>         ../drivers/net/qede/base/ecore_mcp.c:638:2,
>     inlined from ‘ecore_mcp_cmd_and_union’ at
>         ../drivers/net/qede/base/ecore_mcp.c:742:9:
> /usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
>         array subscript 1 is outside array bounds of
>         ‘union drv_union_data[1]’ [-Werror=array-bounds]
>   935 |   *__P = __A;
>       |   ~~~~~^~~~~
> ../drivers/net/qede/base/ecore_mcp.c: In function
>         ‘ecore_mcp_cmd_and_union’:
> ../drivers/net/qede/base/ecore_mcp.c:533:30: note: at offset 32 into
>         object ‘union_data’ of size 32
>   533 |         union drv_union_data union_data;
>       |                              ^~~~~~~~~~
> 
> Since this code is in the base driver, waive the check until the base
> driver is fixed by the relevant people.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Or mark driver broken with gcc-12 and get the maintainer to fix?

Re: [PATCH 08/12] net/qede/base: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:53 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC raises the following warning:
> 
> In function ‘_mm256_storeu_si256’,
>     inlined from ‘rte_mov32’ at
>         ../lib/eal/x86/include/rte_memcpy.h:320:2,
>     inlined from ‘rte_mov128’ at
>         ../lib/eal/x86/include/rte_memcpy.h:342: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 ‘__ecore_mcp_cmd_and_union’ at
>         ../drivers/net/qede/base/ecore_mcp.c:541:3,
>     inlined from ‘_ecore_mcp_cmd_and_union’ at
>         ../drivers/net/qede/base/ecore_mcp.c:638:2,
>     inlined from ‘ecore_mcp_cmd_and_union’ at
>         ../drivers/net/qede/base/ecore_mcp.c:742:9:
> /usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
>         array subscript 1 is outside array bounds of
>         ‘union drv_union_data[1]’ [-Werror=array-bounds]
>   935 |   *__P = __A;
>       |   ~~~~~^~~~~
> ../drivers/net/qede/base/ecore_mcp.c: In function
>         ‘ecore_mcp_cmd_and_union’:
> ../drivers/net/qede/base/ecore_mcp.c:533:30: note: at offset 32 into
>         object ‘union_data’ of size 32
>   533 |         union drv_union_data union_data;
>       |                              ^~~~~~~~~~
> 
> Since this code is in the base driver, waive the check until the base
> driver is fixed by the relevant people.

Even there are two maintainers, haven't heard a response from them.
It could be a real bug.

Re: [PATCH 08/12] net/qede/base: fix build with GCC 12

[David Marchand]

On Wed, Jun 22, 2022 at 1:17 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
> > GCC raises the following warning:
> >
> > In function ‘_mm256_storeu_si256’,
> >     inlined from ‘rte_mov32’ at
> >         ../lib/eal/x86/include/rte_memcpy.h:320:2,
> >     inlined from ‘rte_mov128’ at
> >         ../lib/eal/x86/include/rte_memcpy.h:342: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 ‘__ecore_mcp_cmd_and_union’ at
> >         ../drivers/net/qede/base/ecore_mcp.c:541:3,
> >     inlined from ‘_ecore_mcp_cmd_and_union’ at
> >         ../drivers/net/qede/base/ecore_mcp.c:638:2,
> >     inlined from ‘ecore_mcp_cmd_and_union’ at
> >         ../drivers/net/qede/base/ecore_mcp.c:742:9:
> > /usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
> >         array subscript 1 is outside array bounds of
> >         ‘union drv_union_data[1]’ [-Werror=array-bounds]
> >   935 |   *__P = __A;
> >       |   ~~~~~^~~~~
> > ../drivers/net/qede/base/ecore_mcp.c: In function
> >         ‘ecore_mcp_cmd_and_union’:
> > ../drivers/net/qede/base/ecore_mcp.c:533:30: note: at offset 32 into
> >         object ‘union_data’ of size 32
> >   533 |         union drv_union_data union_data;
> >       |                              ^~~~~~~~~~
> >
> > Since this code is in the base driver, waive the check until the base
> > driver is fixed by the relevant people.
>
> Even there are two maintainers, haven't heard a response from them.
> It could be a real bug.

Maintainers were pinged privately but I see no progress.
If I don't get a reply from them by tomorrow morning (GMT+2), I will
merge your RFC patch as it seems the best fix atm.

https://patchwork.dpdk.org/project/dpdk/patch/20220607171746.461772-3-stephen@networkplumber.org/


-- 
David Marchand

[PATCH 09/12] vdpa/ifc: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

../drivers/vdpa/ifc/ifcvf_vdpa.c: In function ‘vdpa_enable_vfio_intr’:
../drivers/vdpa/ifc/ifcvf_vdpa.c:383:62: error: writing 4 bytes into a
    region of size 0 [-Werror=stringop-overflow=]
  383 |                         fd_ptr[RTE_INTR_VEC_RXTX_OFFSET + i] = fd;
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
../drivers/vdpa/ifc/ifcvf_vdpa.c:348:14: note: at offset 32 into
    destination object ‘irq_set_buf’ of size 32
  348 |         char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
      |              ^~~~~~~~~~~

Validate number of vrings to avoid out of bound access.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/vdpa/ifc/ifcvf_vdpa.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
index 9f05595b6b..6708849bd3 100644
--- a/drivers/vdpa/ifc/ifcvf_vdpa.c
+++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
@@ -354,6 +354,8 @@ vdpa_enable_vfio_intr(struct ifcvf_internal *internal, bool m_rx)
 	vring.callfd = -1;
 
 	nr_vring = rte_vhost_get_vring_num(internal->vid);
+	if (nr_vring > IFCVF_MAX_QUEUES * 2)
+		return -1;
 
 	irq_set = (struct vfio_irq_set *)irq_set_buf;
 	irq_set->argsz = sizeof(irq_set_buf);
-- 
2.36.1

RE: [PATCH 09/12] vdpa/ifc: fix build with GCC 12

[Wang, Xiao W]

Hi,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, May 18, 2022 6:17 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; ferruh.yigit@xilinx.com; stable@dpdk.org;
> Wang, Xiao W <xiao.w.wang@intel.com>
> Subject: [PATCH 09/12] vdpa/ifc: fix build with GCC 12
> 
> GCC 12 raises the following warning:
> 
> ../drivers/vdpa/ifc/ifcvf_vdpa.c: In function ‘vdpa_enable_vfio_intr’:
> ../drivers/vdpa/ifc/ifcvf_vdpa.c:383:62: error: writing 4 bytes into a
>     region of size 0 [-Werror=stringop-overflow=]
>   383 |                         fd_ptr[RTE_INTR_VEC_RXTX_OFFSET + i] = fd;
>       |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ../drivers/vdpa/ifc/ifcvf_vdpa.c:348:14: note: at offset 32 into
>     destination object ‘irq_set_buf’ of size 32
>   348 |         char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
>       |              ^~~~~~~~~~~
> 
> Validate number of vrings to avoid out of bound access.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/vdpa/ifc/ifcvf_vdpa.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
> index 9f05595b6b..6708849bd3 100644
> --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
> +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
> @@ -354,6 +354,8 @@ vdpa_enable_vfio_intr(struct ifcvf_internal *internal,
> bool m_rx)
>  	vring.callfd = -1;
> 
>  	nr_vring = rte_vhost_get_vring_num(internal->vid);
> +	if (nr_vring > IFCVF_MAX_QUEUES * 2)
> +		return -1;
> 
>  	irq_set = (struct vfio_irq_set *)irq_set_buf;
>  	irq_set->argsz = sizeof(irq_set_buf);
> --
> 2.36.1

Acked-by: Xiao Wang <xiao.w.wang@intel.com>

BRs,
Xiao

Re: [PATCH 09/12] vdpa/ifc: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:54 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> ../drivers/vdpa/ifc/ifcvf_vdpa.c: In function ‘vdpa_enable_vfio_intr’:
> ../drivers/vdpa/ifc/ifcvf_vdpa.c:383:62: error: writing 4 bytes into a
>     region of size 0 [-Werror=stringop-overflow=]
>   383 |                         fd_ptr[RTE_INTR_VEC_RXTX_OFFSET + i] = fd;
>       |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
> ../drivers/vdpa/ifc/ifcvf_vdpa.c:348:14: note: at offset 32 into
>     destination object ‘irq_set_buf’ of size 32
>   348 |         char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
>       |              ^~~~~~~~~~~
> 
> Validate number of vrings to avoid out of bound access.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 10/12] vhost/crypto: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In file included from ../lib/mempool/rte_mempool.h:46,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/vhost/vhost_crypto.c:7:
../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
     outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
     [-Warray-bounds]
  371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
 1178 |         struct virtio_crypto_op_data_req req;
      |                                          ^~~

Check that copied length is within req boundaries.

Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/vhost_crypto.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
index b1c0eb6a0f..83325b7042 100644
--- a/lib/vhost/vhost_crypto.c
+++ b/lib/vhost/vhost_crypto.c
@@ -576,16 +576,16 @@ copy_data(void *dst_data, struct vhost_crypto_data_req *vc_req,
 	uint32_t to_copy;
 	uint8_t *data = dst_data;
 	uint8_t *src;
-	int left = size;
+	uint32_t left = size;
 
-	to_copy = RTE_MIN(desc->len, (uint32_t)left);
+	to_copy = RTE_MIN(desc->len, left);
 	dlen = to_copy;
 	src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
 			VHOST_ACCESS_RO);
-	if (unlikely(!src || !dlen))
+	if (unlikely(!src || !dlen || dlen > left))
 		return -1;
 
-	rte_memcpy((uint8_t *)data, src, dlen);
+	rte_memcpy(data, src, dlen);
 	data += dlen;
 
 	if (unlikely(dlen < to_copy)) {
-- 
2.36.1

Re: [PATCH 10/12] vhost/crypto: fix build with GCC 12

[Bruce Richardson]

On Wed, May 18, 2022 at 12:16:55PM +0200, David Marchand wrote:
> GCC 12 raises the following warning:
> 
> In file included from ../lib/mempool/rte_mempool.h:46,
>                  from ../lib/mbuf/rte_mbuf.h:38,
>                  from ../lib/vhost/vhost_crypto.c:7:
> ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
>      outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
>      [-Warray-bounds]
>   371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
>  1178 |         struct virtio_crypto_op_data_req req;
>       |                                          ^~~
> 
> Check that copied length is within req boundaries.
> 
> Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/vhost/vhost_crypto.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
> index b1c0eb6a0f..83325b7042 100644
> --- a/lib/vhost/vhost_crypto.c
> +++ b/lib/vhost/vhost_crypto.c
> @@ -576,16 +576,16 @@ copy_data(void *dst_data, struct vhost_crypto_data_req *vc_req,
>  	uint32_t to_copy;
>  	uint8_t *data = dst_data;
>  	uint8_t *src;
> -	int left = size;
> +	uint32_t left = size;
>  
> -	to_copy = RTE_MIN(desc->len, (uint32_t)left);
> +	to_copy = RTE_MIN(desc->len, left);
>  	dlen = to_copy;
>  	src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
>  			VHOST_ACCESS_RO);

Tracking the functions which end up being called by this macro, the dlen
parameter ends up being of type "uint64_t *", passing a value of int * or
uint32_t * seems wrong to me. If we are changing the type from int to
uint32_t, I think it should be promoted all the way to uint64_t.

> -	if (unlikely(!src || !dlen))
> +	if (unlikely(!src || !dlen || dlen > left))
>  		return -1;
>  

If this change is omitted, does the compiler still give warnings. Looking
through the called code, the dlen parameter can only ever be reduced, not
incremented (function rte_vhost_va_from_guest_pa() in rte_vhost.h).

> -	rte_memcpy((uint8_t *)data, src, dlen);
> +	rte_memcpy(data, src, dlen);
>  	data += dlen;
>  
>  	if (unlikely(dlen < to_copy)) {
> -- 
> 2.36.1
> 

Re: [PATCH 10/12] vhost/crypto: fix build with GCC 12

[David Marchand]

On Thu, Jun 2, 2022 at 12:09 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, May 18, 2022 at 12:16:55PM +0200, David Marchand wrote:
> > GCC 12 raises the following warning:
> >
> > In file included from ../lib/mempool/rte_mempool.h:46,
> >                  from ../lib/mbuf/rte_mbuf.h:38,
> >                  from ../lib/vhost/vhost_crypto.c:7:
> > ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> > ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
> >      outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
> >      [-Warray-bounds]
> >   371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
> >       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
> >  1178 |         struct virtio_crypto_op_data_req req;
> >       |                                          ^~~
> >
> > Check that copied length is within req boundaries.
> >
> > Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  lib/vhost/vhost_crypto.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
> > index b1c0eb6a0f..83325b7042 100644
> > --- a/lib/vhost/vhost_crypto.c
> > +++ b/lib/vhost/vhost_crypto.c
> > @@ -576,16 +576,16 @@ copy_data(void *dst_data, struct vhost_crypto_data_req *vc_req,
> >       uint32_t to_copy;
> >       uint8_t *data = dst_data;
> >       uint8_t *src;
> > -     int left = size;
> > +     uint32_t left = size;
> >
> > -     to_copy = RTE_MIN(desc->len, (uint32_t)left);
> > +     to_copy = RTE_MIN(desc->len, left);
> >       dlen = to_copy;
> >       src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
> >                       VHOST_ACCESS_RO);
>
> Tracking the functions which end up being called by this macro, the dlen
> parameter ends up being of type "uint64_t *", passing a value of int * or
> uint32_t * seems wrong to me. If we are changing the type from int to
> uint32_t, I think it should be promoted all the way to uint64_t.

Indeed.
I'll update in v2.

We already had some CVE on this part of the code, a careful review is needed.


>
> > -     if (unlikely(!src || !dlen))
> > +     if (unlikely(!src || !dlen || dlen > left))
> >               return -1;
> >
>
> If this change is omitted, does the compiler still give warnings. Looking
> through the called code, the dlen parameter can only ever be reduced, not
> incremented (function rte_vhost_va_from_guest_pa() in rte_vhost.h).

If I promote to_copy and left variables as uint64_t, gcc is still
unhappy, for the same reason.
The check on dlen > left seems necessary.


>
> > -     rte_memcpy((uint8_t *)data, src, dlen);
> > +     rte_memcpy(data, src, dlen);
> >       data += dlen;
> >
> >       if (unlikely(dlen < to_copy)) {
> > --
> > 2.36.1
> >
>

-- 
David Marchand

Re: [PATCH 10/12] vhost/crypto: fix build with GCC 12

[Bruce Richardson]

On Tue, Jun 14, 2022 at 11:22:24AM +0200, David Marchand wrote:
> On Thu, Jun 2, 2022 at 12:09 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Wed, May 18, 2022 at 12:16:55PM +0200, David Marchand wrote:
> > > GCC 12 raises the following warning:
> > >
> > > In file included from ../lib/mempool/rte_mempool.h:46,
> > >                  from ../lib/mbuf/rte_mbuf.h:38,
> > >                  from ../lib/vhost/vhost_crypto.c:7:
> > > ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> > > ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
> > >      outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
> > >      [-Warray-bounds]
> > >   371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
> > >       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
> > >  1178 |         struct virtio_crypto_op_data_req req;
> > >       |                                          ^~~
> > >
> > > Check that copied length is within req boundaries.
> > >
> > > Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > >  lib/vhost/vhost_crypto.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
> > > index b1c0eb6a0f..83325b7042 100644
> > > --- a/lib/vhost/vhost_crypto.c
> > > +++ b/lib/vhost/vhost_crypto.c
> > > @@ -576,16 +576,16 @@ copy_data(void *dst_data, struct vhost_crypto_data_req *vc_req,
> > >       uint32_t to_copy;
> > >       uint8_t *data = dst_data;
> > >       uint8_t *src;
> > > -     int left = size;
> > > +     uint32_t left = size;
> > >
> > > -     to_copy = RTE_MIN(desc->len, (uint32_t)left);
> > > +     to_copy = RTE_MIN(desc->len, left);
> > >       dlen = to_copy;
> > >       src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
> > >                       VHOST_ACCESS_RO);
> >
> > Tracking the functions which end up being called by this macro, the dlen
> > parameter ends up being of type "uint64_t *", passing a value of int * or
> > uint32_t * seems wrong to me. If we are changing the type from int to
> > uint32_t, I think it should be promoted all the way to uint64_t.
> 
> Indeed.
> I'll update in v2.
> 
> We already had some CVE on this part of the code, a careful review is needed.
> 
> 
> >
> > > -     if (unlikely(!src || !dlen))
> > > +     if (unlikely(!src || !dlen || dlen > left))
> > >               return -1;
> > >
> >
> > If this change is omitted, does the compiler still give warnings. Looking
> > through the called code, the dlen parameter can only ever be reduced, not
> > incremented (function rte_vhost_va_from_guest_pa() in rte_vhost.h).
> 
> If I promote to_copy and left variables as uint64_t, gcc is still
> unhappy, for the same reason.
> The check on dlen > left seems necessary.
> 
> 
Ok, just thought I'd ask anyway. I wonder if we need to check for
wrap-around in the reduction case, since we are dealing with unsigned
values. This additional check should catch that anyway if it does occur.

/Bruce

Re: [PATCH 10/12] vhost/crypto: fix build with GCC 12

[David Marchand]

On Tue, Jun 14, 2022 at 11:25 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > > > -     if (unlikely(!src || !dlen))
> > > > +     if (unlikely(!src || !dlen || dlen > left))
> > > >               return -1;
> > > >
> > >
> > > If this change is omitted, does the compiler still give warnings. Looking
> > > through the called code, the dlen parameter can only ever be reduced, not
> > > incremented (function rte_vhost_va_from_guest_pa() in rte_vhost.h).
> >
> > If I promote to_copy and left variables as uint64_t, gcc is still
> > unhappy, for the same reason.
> > The check on dlen > left seems necessary.
> >
> >
> Ok, just thought I'd ask anyway. I wonder if we need to check for
> wrap-around in the reduction case, since we are dealing with unsigned
> values. This additional check should catch that anyway if it does occur.

I had a fresh look at this code and went with some splitting / simplification.
This makes the code clearer, and there is no added check.

I'll send a v2.


-- 
David Marchand

Re: [PATCH 10/12] vhost/crypto: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:55 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> In file included from ../lib/mempool/rte_mempool.h:46,
>                  from ../lib/mbuf/rte_mbuf.h:38,
>                  from ../lib/vhost/vhost_crypto.c:7:
> ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
>      outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
>      [-Warray-bounds]
>   371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
>  1178 |         struct virtio_crypto_op_data_req req;
>       |                                          ^~~
> 
> Check that copied length is within req boundaries.
> 
> Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH v2] vhost/crypto: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In file included from ../lib/mempool/rte_mempool.h:46,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/vhost/vhost_crypto.c:7:
../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
     outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
     [-Warray-bounds]
  371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
 1178 |         struct virtio_crypto_op_data_req req;
      |                                          ^~~

Split this function and separate the per descriptor copy.
This makes the code clearer, and the compiler happier.

Note: logs for errors have been moved to callers to avoid duplicates.

Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- refactored copy function,

---
 lib/vhost/vhost_crypto.c | 122 +++++++++++++++------------------------
 1 file changed, 45 insertions(+), 77 deletions(-)

diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
index b1c0eb6a0f..1bc42896ea 100644
--- a/lib/vhost/vhost_crypto.c
+++ b/lib/vhost/vhost_crypto.c
@@ -565,94 +565,57 @@ get_data_ptr(struct vhost_crypto_data_req *vc_req,
 	return data;
 }
 
-static __rte_always_inline int
-copy_data(void *dst_data, struct vhost_crypto_data_req *vc_req,
-		struct vhost_crypto_desc *head,
-		struct vhost_crypto_desc **cur_desc,
-		uint32_t size, uint32_t max_n_descs)
+static __rte_always_inline uint32_t
+copy_data_from_desc(void *dst, struct vhost_crypto_data_req *vc_req,
+	struct vhost_crypto_desc *desc, uint32_t size)
 {
-	struct vhost_crypto_desc *desc = *cur_desc;
-	uint64_t remain, addr, dlen, len;
-	uint32_t to_copy;
-	uint8_t *data = dst_data;
-	uint8_t *src;
-	int left = size;
-
-	to_copy = RTE_MIN(desc->len, (uint32_t)left);
-	dlen = to_copy;
-	src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
-			VHOST_ACCESS_RO);
-	if (unlikely(!src || !dlen))
-		return -1;
+	uint64_t remain;
+	uint64_t addr;
+
+	remain = RTE_MIN(desc->len, size);
+	addr = desc->addr;
+	do {
+		uint64_t len;
+		void *src;
+
+		len = remain;
+		src = IOVA_TO_VVA(void *, vc_req, addr, &len, VHOST_ACCESS_RO);
+		if (unlikely(src == NULL || len == 0))
+			return 0;
 
-	rte_memcpy((uint8_t *)data, src, dlen);
-	data += dlen;
+		rte_memcpy(dst, src, len);
+		remain -= len;
+		dst = RTE_PTR_ADD(dst, len);
+		addr += len;
+	} while (unlikely(remain != 0));
 
-	if (unlikely(dlen < to_copy)) {
-		remain = to_copy - dlen;
-		addr = desc->addr + dlen;
+	return RTE_MIN(desc->len, size);
+}
 
-		while (remain) {
-			len = remain;
-			src = IOVA_TO_VVA(uint8_t *, vc_req, addr, &len,
-					VHOST_ACCESS_RO);
-			if (unlikely(!src || !len)) {
-				VC_LOG_ERR("Failed to map descriptor");
-				return -1;
-			}
 
-			rte_memcpy(data, src, len);
-			addr += len;
-			remain -= len;
-			data += len;
-		}
-	}
+static __rte_always_inline int
+copy_data(void *data, struct vhost_crypto_data_req *vc_req,
+	struct vhost_crypto_desc *head, struct vhost_crypto_desc **cur_desc,
+	uint32_t size, uint32_t max_n_descs)
+{
+	struct vhost_crypto_desc *desc = *cur_desc;
+	uint32_t left = size;
 
-	left -= to_copy;
+	do {
+		uint32_t copied;
 
-	while (desc >= head && desc - head < (int)max_n_descs && left) {
-		desc++;
-		to_copy = RTE_MIN(desc->len, (uint32_t)left);
-		dlen = to_copy;
-		src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
-				VHOST_ACCESS_RO);
-		if (unlikely(!src || !dlen)) {
-			VC_LOG_ERR("Failed to map descriptor");
+		copied = copy_data_from_desc(data, vc_req, desc, left);
+		if (copied == 0)
 			return -1;
-		}
-
-		rte_memcpy(data, src, dlen);
-		data += dlen;
-
-		if (unlikely(dlen < to_copy)) {
-			remain = to_copy - dlen;
-			addr = desc->addr + dlen;
-
-			while (remain) {
-				len = remain;
-				src = IOVA_TO_VVA(uint8_t *, vc_req, addr, &len,
-						VHOST_ACCESS_RO);
-				if (unlikely(!src || !len)) {
-					VC_LOG_ERR("Failed to map descriptor");
-					return -1;
-				}
-
-				rte_memcpy(data, src, len);
-				addr += len;
-				remain -= len;
-				data += len;
-			}
-		}
-
-		left -= to_copy;
-	}
+		left -= copied;
+		data = RTE_PTR_ADD(data, copied);
+		desc++;
+	} while (desc < head + max_n_descs && left != 0);
 
-	if (unlikely(left > 0)) {
-		VC_LOG_ERR("Incorrect virtio descriptor");
+	if (unlikely(left != 0))
 		return -1;
-	}
 
-	if (unlikely(desc - head == (int)max_n_descs))
+	if (unlikely(desc == head + max_n_descs))
 		*cur_desc = NULL;
 	else
 		*cur_desc = desc + 1;
@@ -852,6 +815,7 @@ prepare_sym_cipher_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 	/* iv */
 	if (unlikely(copy_data(iv_data, vc_req, head, &desc,
 			cipher->para.iv_len, max_n_descs))) {
+		VC_LOG_ERR("Incorrect virtio descriptor");
 		ret = VIRTIO_CRYPTO_BADMSG;
 		goto error_exit;
 	}
@@ -883,6 +847,7 @@ prepare_sym_cipher_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 		if (unlikely(copy_data(rte_pktmbuf_mtod(m_src, uint8_t *),
 				vc_req, head, &desc, cipher->para.src_data_len,
 				max_n_descs) < 0)) {
+			VC_LOG_ERR("Incorrect virtio descriptor");
 			ret = VIRTIO_CRYPTO_BADMSG;
 			goto error_exit;
 		}
@@ -1006,6 +971,7 @@ prepare_sym_chain_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 	/* iv */
 	if (unlikely(copy_data(iv_data, vc_req, head, &desc,
 			chain->para.iv_len, max_n_descs) < 0)) {
+		VC_LOG_ERR("Incorrect virtio descriptor");
 		ret = VIRTIO_CRYPTO_BADMSG;
 		goto error_exit;
 	}
@@ -1037,6 +1003,7 @@ prepare_sym_chain_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 		if (unlikely(copy_data(rte_pktmbuf_mtod(m_src, uint8_t *),
 				vc_req, head, &desc, chain->para.src_data_len,
 				max_n_descs) < 0)) {
+			VC_LOG_ERR("Incorrect virtio descriptor");
 			ret = VIRTIO_CRYPTO_BADMSG;
 			goto error_exit;
 		}
@@ -1121,6 +1088,7 @@ prepare_sym_chain_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 		if (unlikely(copy_data(digest_addr, vc_req, head, &digest_desc,
 				chain->para.hash_result_len,
 				max_n_descs) < 0)) {
+			VC_LOG_ERR("Incorrect virtio descriptor");
 			ret = VIRTIO_CRYPTO_BADMSG;
 			goto error_exit;
 		}
-- 
2.36.1

Re: [PATCH v2] vhost/crypto: fix build with GCC 12

[David Marchand]

On Thu, Jun 16, 2022 at 11:32 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> GCC 12 raises the following warning:
>
> In file included from ../lib/mempool/rte_mempool.h:46,
>                  from ../lib/mbuf/rte_mbuf.h:38,
>                  from ../lib/vhost/vhost_crypto.c:7:
> ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
>      outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
>      [-Warray-bounds]
>   371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
>  1178 |         struct virtio_crypto_op_data_req req;
>       |                                          ^~~
>
> Split this function and separate the per descriptor copy.
> This makes the code clearer, and the compiler happier.
>
> Note: logs for errors have been moved to callers to avoid duplicates.
>
> Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Build fails on 32-bit.
I had seen the issue while testing but sent a non amended patch... v3 incoming.

-- 
David Marchand

[PATCH 11/12] app/flow-perf: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

../app/test-flow-perf/main.c: In function ‘start_forwarding’:
../app/test-flow-perf/main.c:1737:28: error: ‘sprintf’ may write a
    terminating nul past the end of the destination
    [-Werror=format-overflow=]
 1737 |         sprintf(p[i++], "%d", (int)n);
      |                            ^
In function ‘pretty_number’,
    inlined from ‘packet_per_second_stats’ at
        ../app/test-flow-perf/main.c:1792:4,
    inlined from ‘start_forwarding’ at
        ../app/test-flow-perf/main.c:1831:3:
[...]

We can simplify this code and rely on libc integer formatting via
this system locales.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test-flow-perf/main.c | 48 ++++++++-------------------------------
 1 file changed, 9 insertions(+), 39 deletions(-)

diff --git a/app/test-flow-perf/main.c b/app/test-flow-perf/main.c
index 56d43734e3..3922e92ded 100644
--- a/app/test-flow-perf/main.c
+++ b/app/test-flow-perf/main.c
@@ -16,6 +16,7 @@
  * gives packet per second measurement.
  */
 
+#include <locale.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -1713,36 +1714,6 @@ do_tx(struct lcore_info *li, uint16_t cnt, uint16_t tx_port,
 		rte_pktmbuf_free(li->pkts[i]);
 }
 
-/*
- * Method to convert numbers into pretty numbers that easy
- * to read. The design here is to add comma after each three
- * digits and set all of this inside buffer.
- *
- * For example if n = 1799321, the output will be
- * 1,799,321 after this method which is easier to read.
- */
-static char *
-pretty_number(uint64_t n, char *buf)
-{
-	char p[6][4];
-	int i = 0;
-	int off = 0;
-
-	while (n > 1000) {
-		sprintf(p[i], "%03d", (int)(n % 1000));
-		n /= 1000;
-		i += 1;
-	}
-
-	sprintf(p[i++], "%d", (int)n);
-
-	while (i--)
-		off += sprintf(buf + off, "%s,", p[i]);
-	buf[strlen(buf) - 1] = '\0';
-
-	return buf;
-}
-
 static void
 packet_per_second_stats(void)
 {
@@ -1764,7 +1735,6 @@ packet_per_second_stats(void)
 		uint64_t total_rx_pkts = 0;
 		uint64_t total_tx_drops = 0;
 		uint64_t tx_delta, rx_delta, drops_delta;
-		char buf[3][32];
 		int nr_valid_core = 0;
 
 		sleep(1);
@@ -1789,10 +1759,8 @@ packet_per_second_stats(void)
 			tx_delta    = li->tx_pkts  - oli->tx_pkts;
 			rx_delta    = li->rx_pkts  - oli->rx_pkts;
 			drops_delta = li->tx_drops - oli->tx_drops;
-			printf("%6d %16s %16s %16s\n", i,
-				pretty_number(tx_delta,    buf[0]),
-				pretty_number(drops_delta, buf[1]),
-				pretty_number(rx_delta,    buf[2]));
+			printf("%6d %'16.3"PRId64" %'16.3"PRId64" %'16.3"PRId64"\n",
+				i, tx_delta, drops_delta, rx_delta);
 
 			total_tx_pkts  += tx_delta;
 			total_rx_pkts  += rx_delta;
@@ -1803,10 +1771,9 @@ packet_per_second_stats(void)
 		}
 
 		if (nr_valid_core > 1) {
-			printf("%6s %16s %16s %16s\n", "total",
-				pretty_number(total_tx_pkts,  buf[0]),
-				pretty_number(total_tx_drops, buf[1]),
-				pretty_number(total_rx_pkts,  buf[2]));
+			printf("%6s %'16.3"PRId64" %'16.3"PRId64" %'16.3"PRId64"\n",
+				"total", total_tx_pkts, total_tx_drops,
+				total_rx_pkts);
 			nr_lines += 1;
 		}
 
@@ -2139,6 +2106,9 @@ main(int argc, char **argv)
 	if (argc > 1)
 		args_parse(argc, argv);
 
+	/* For more fancy, localised integer formatting. */
+	setlocale(LC_NUMERIC, "");
+
 	init_port();
 
 	nb_lcores = rte_lcore_count();
-- 
2.36.1

Re: [PATCH 11/12] app/flow-perf: fix build with GCC 12

[Bruce Richardson]

On Wed, May 18, 2022 at 12:16:56PM +0200, David Marchand wrote:
> GCC 12 raises the following warning:
> 
> ../app/test-flow-perf/main.c: In function ‘start_forwarding’:
> ../app/test-flow-perf/main.c:1737:28: error: ‘sprintf’ may write a
>     terminating nul past the end of the destination
>     [-Werror=format-overflow=]
>  1737 |         sprintf(p[i++], "%d", (int)n);
>       |                            ^
> In function ‘pretty_number’,
>     inlined from ‘packet_per_second_stats’ at
>         ../app/test-flow-perf/main.c:1792:4,
>     inlined from ‘start_forwarding’ at
>         ../app/test-flow-perf/main.c:1831:3:
> [...]
> 
> We can simplify this code and rely on libc integer formatting via
> this system locales.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Good idea.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

RE: [PATCH 11/12] app/flow-perf: fix build with GCC 12

[Wisam Monther]

Hi David,

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, May 18, 2022 1:17 PM
> To: dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> ferruh.yigit@xilinx.com; stable@dpdk.org; Wisam Monther
> <wisamm@nvidia.com>
> Subject: [PATCH 11/12] app/flow-perf: fix build with GCC 12
> 
> GCC 12 raises the following warning:
> 
> ../app/test-flow-perf/main.c: In function ‘start_forwarding’:
> ../app/test-flow-perf/main.c:1737:28: error: ‘sprintf’ may write a
>     terminating nul past the end of the destination
>     [-Werror=format-overflow=]
>  1737 |         sprintf(p[i++], "%d", (int)n);
>       |                            ^
> In function ‘pretty_number’,
>     inlined from ‘packet_per_second_stats’ at
>         ../app/test-flow-perf/main.c:1792:4,
>     inlined from ‘start_forwarding’ at
>         ../app/test-flow-perf/main.c:1831:3:
> [...]
> 
> We can simplify this code and rely on libc integer formatting via this system
> locales.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

I've tested the patch and reviewed it, it's working fine, so thank you for that.
One comment
The initial value of 0 is 000

Example:
CMD: ./dpdk-test-flow-perf -n 4 -a <PCI> -- ingress --group=1 --ether --queue --rules-count=200000 --enable-fwd
  core               tx         tx drops               rx
------ ---------------- ---------------- ----------------
     1              000              000              000

Can you handle this to be single 0 instead of not needed leading zeros? 

BRs,
Wisam Jaddo

Re: [PATCH 11/12] app/flow-perf: fix build with GCC 12

[David Marchand]

On Wed, Jun 8, 2022 at 11:03 AM Wisam Monther <wisamm@nvidia.com> wrote:
>
> Hi David,
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Wednesday, May 18, 2022 1:17 PM
> > To: dev@dpdk.org
> > Cc: NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> > ferruh.yigit@xilinx.com; stable@dpdk.org; Wisam Monther
> > <wisamm@nvidia.com>
> > Subject: [PATCH 11/12] app/flow-perf: fix build with GCC 12
> >
> > GCC 12 raises the following warning:
> >
> > ../app/test-flow-perf/main.c: In function ‘start_forwarding’:
> > ../app/test-flow-perf/main.c:1737:28: error: ‘sprintf’ may write a
> >     terminating nul past the end of the destination
> >     [-Werror=format-overflow=]
> >  1737 |         sprintf(p[i++], "%d", (int)n);
> >       |                            ^
> > In function ‘pretty_number’,
> >     inlined from ‘packet_per_second_stats’ at
> >         ../app/test-flow-perf/main.c:1792:4,
> >     inlined from ‘start_forwarding’ at
> >         ../app/test-flow-perf/main.c:1831:3:
> > [...]
> >
> > We can simplify this code and rely on libc integer formatting via this system
> > locales.
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> I've tested the patch and reviewed it, it's working fine, so thank you for that.
> One comment
> The initial value of 0 is 000
>
> Example:
> CMD: ./dpdk-test-flow-perf -n 4 -a <PCI> -- ingress --group=1 --ether --queue --rules-count=200000 --enable-fwd
>   core               tx         tx drops               rx
> ------ ---------------- ---------------- ----------------
>      1              000              000              000
>
> Can you handle this to be single 0 instead of not needed leading zeros?

Hum, I don't remember why I added this precision...
This should be just a matter of changing the format from %'16.3s to
%'16s, can you confirm?

-- 
David Marchand

RE: [PATCH 11/12] app/flow-perf: fix build with GCC 12

[Wisam Monther]

Hi,

> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Wednesday, May 18, 2022 1:17 PM
> > > To: dev@dpdk.org
> > > Cc: NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>;
> > > ferruh.yigit@xilinx.com; stable@dpdk.org; Wisam Monther
> > > <wisamm@nvidia.com>
> > > Subject: [PATCH 11/12] app/flow-perf: fix build with GCC 12
> > >
> > > GCC 12 raises the following warning:
> > >
> > > ../app/test-flow-perf/main.c: In function ‘start_forwarding’:
> > > ../app/test-flow-perf/main.c:1737:28: error: ‘sprintf’ may write a
> > >     terminating nul past the end of the destination
> > >     [-Werror=format-overflow=]
> > >  1737 |         sprintf(p[i++], "%d", (int)n);
> > >       |                            ^
> > > In function ‘pretty_number’,
> > >     inlined from ‘packet_per_second_stats’ at
> > >         ../app/test-flow-perf/main.c:1792:4,
> > >     inlined from ‘start_forwarding’ at
> > >         ../app/test-flow-perf/main.c:1831:3:
> > > [...]
> > >
> > > We can simplify this code and rely on libc integer formatting via
> > > this system locales.
> > >
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> >
> > I've tested the patch and reviewed it, it's working fine, so thank you for
> that.
> > One comment
> > The initial value of 0 is 000
> >
> > Example:
> > CMD: ./dpdk-test-flow-perf -n 4 -a <PCI> -- ingress --group=1 --ether --
> queue --rules-count=200000 --enable-fwd
> >   core               tx         tx drops               rx
> > ------ ---------------- ---------------- ----------------
> >      1              000              000              000
> >
> > Can you handle this to be single 0 instead of not needed leading zeros?
> 
> Hum, I don't remember why I added this precision...
> This should be just a matter of changing the format from %'16.3s to %'16s,
> can you confirm?

Confirmed, you can go with it. Thanks in advance.

BRs,
Wisam Jaddo

Re: [PATCH 11/12] app/flow-perf: fix build with GCC 12

[Stephen Hemminger]

On Wed, 18 May 2022 12:16:56 +0200
David Marchand <david.marchand@redhat.com> wrote:

> GCC 12 raises the following warning:
> 
> ../app/test-flow-perf/main.c: In function ‘start_forwarding’:
> ../app/test-flow-perf/main.c:1737:28: error: ‘sprintf’ may write a
>     terminating nul past the end of the destination
>     [-Werror=format-overflow=]
>  1737 |         sprintf(p[i++], "%d", (int)n);
>       |                            ^
> In function ‘pretty_number’,
>     inlined from ‘packet_per_second_stats’ at
>         ../app/test-flow-perf/main.c:1792:4,
>     inlined from ‘start_forwarding’ at
>         ../app/test-flow-perf/main.c:1831:3:
> [...]
> 
> We can simplify this code and rely on libc integer formatting via
> this system locales.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Fixes and ends up cleaner.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 12/12] test/ipsec: fix build with GCC 12

[David Marchand]

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)
+				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;
 	}
-- 
2.36.1

Re: [PATCH 12/12] test/ipsec: fix build with GCC 12

[Medvedkin, Vladimir]

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;
>   	}

-- 
Regards,
Vladimir

Re: [PATCH 12/12] test/ipsec: fix build with GCC 12

[David Marchand]

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);


-- 
David Marchand

Re: [PATCH 12/12] test/ipsec: fix build with GCC 12

[Bruce Richardson]

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?

Re: [PATCH 12/12] test/ipsec: fix build with GCC 12

[David Marchand]

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);


-- 
David Marchand

Re: [PATCH 12/12] test/ipsec: fix build with GCC 12

[Medvedkin, Vladimir]

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);
> 
> 

-- 
Regards,
Vladimir

Re: [PATCH 12/12] test/ipsec: fix build with GCC 12

[Stephen Hemminger]

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.

[PATCH v2] test/ipsec: fix build with GCC 12

[David Marchand]

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[] =
      |            ^~~~~~~~~~~~~~~

Add a hint so that the compiler understands the copied data is within
the passed string boundaries.

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- let the code as is, simply added a RTE_VERIFY hint,

---
 app/test/test_ipsec.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/app/test/test_ipsec.c b/app/test/test_ipsec.c
index 8da025bf66..7047e17960 100644
--- a/app/test/test_ipsec.c
+++ b/app/test/test_ipsec.c
@@ -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);
 		char *dst = rte_pktmbuf_append(m, t_len);
@@ -1365,7 +1367,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, 0);
 		if (ut_params->ibuf[j] == NULL)
 			rc = TEST_FAILED;
 		else {
@@ -1483,7 +1486,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, 0);
 			if (ut_params->obuf[j] == NULL)
 				rc = TEST_FAILED;
 		}
@@ -1551,16 +1555,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, 0);
 		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, 0);
 			if (ut_params->obuf[j] == NULL)
 				rc = TEST_FAILED;
 		}
@@ -1660,7 +1665,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, 0);
 		if (ut_params->ibuf[0] == NULL)
 			rc = TEST_FAILED;
 
@@ -1738,15 +1744,17 @@ 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, 0);
 		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,
+				0);
 			if (ut_params->obuf[j] == NULL)
 				rc = TEST_FAILED;
 		}
@@ -1815,7 +1823,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, 0);
 		if (ut_params->ibuf[j] == NULL)
 			rc = TEST_FAILED;
 	}
-- 
2.36.1

Re: [PATCH v2] test/ipsec: fix build with GCC 12

[David Marchand]

On Thu, Jun 16, 2022 at 11:33 AM 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[] =
>       |            ^~~~~~~~~~~~~~~
>
> Add a hint so that the compiler understands the copied data is within
> the passed string boundaries.
>

Ferruh had opened a bz.

Bugzilla ID: 848
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

RE: [EXT] Re: [PATCH v2] test/ipsec: fix build with GCC 12

[Akhil Goyal]

> On Thu, Jun 16, 2022 at 11:33 AM 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[] =
> >       |            ^~~~~~~~~~~~~~~
> >
> > Add a hint so that the compiler understands the copied data is within
> > the passed string boundaries.
> >
> 
> Ferruh had opened a bz.
> 
> Bugzilla ID: 848
Added Fixes tag also.

Applied to dpdk-next-crypto

Thanks.

> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>

Re: [PATCH v2] test/ipsec: fix build with GCC 12

[Medvedkin, Vladimir]

On 17/06/2022 13:06, David Marchand wrote:
> On Thu, Jun 16, 2022 at 11:33 AM 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[] =
>>        |            ^~~~~~~~~~~~~~~
>>
>> Add a hint so that the compiler understands the copied data is within
>> the passed string boundaries.
>>
> Ferruh had opened a bz.
>
> Bugzilla ID: 848
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>


Acked-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

>
-- 
Regards,
Vladimir

[PATCH v3] vhost/crypto: fix build with GCC 12

[David Marchand]

GCC 12 raises the following warning:

In file included from ../lib/mempool/rte_mempool.h:46,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/vhost/vhost_crypto.c:7:
../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
     outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
     [-Warray-bounds]
  371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
 1178 |         struct virtio_crypto_op_data_req req;
      |                                          ^~~

Split this function and separate the per descriptor copy.
This makes the code clearer, and the compiler happier.

Note: logs for errors have been moved to callers to avoid duplicates.

Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- fixed 32-bits build,

Changes since v1:
- refactored copy function,

---
 lib/vhost/vhost_crypto.c | 123 +++++++++++++++------------------------
 1 file changed, 46 insertions(+), 77 deletions(-)

diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
index b1c0eb6a0f..96ffb82a5d 100644
--- a/lib/vhost/vhost_crypto.c
+++ b/lib/vhost/vhost_crypto.c
@@ -565,94 +565,58 @@ get_data_ptr(struct vhost_crypto_data_req *vc_req,
 	return data;
 }
 
-static __rte_always_inline int
-copy_data(void *dst_data, struct vhost_crypto_data_req *vc_req,
-		struct vhost_crypto_desc *head,
-		struct vhost_crypto_desc **cur_desc,
-		uint32_t size, uint32_t max_n_descs)
+static __rte_always_inline uint32_t
+copy_data_from_desc(void *dst, struct vhost_crypto_data_req *vc_req,
+	struct vhost_crypto_desc *desc, uint32_t size)
 {
-	struct vhost_crypto_desc *desc = *cur_desc;
-	uint64_t remain, addr, dlen, len;
-	uint32_t to_copy;
-	uint8_t *data = dst_data;
-	uint8_t *src;
-	int left = size;
-
-	to_copy = RTE_MIN(desc->len, (uint32_t)left);
-	dlen = to_copy;
-	src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
-			VHOST_ACCESS_RO);
-	if (unlikely(!src || !dlen))
-		return -1;
+	uint64_t remain;
+	uint64_t addr;
+
+	remain = RTE_MIN(desc->len, size);
+	addr = desc->addr;
+	do {
+		uint64_t len;
+		void *src;
+
+		len = remain;
+		src = IOVA_TO_VVA(void *, vc_req, addr, &len, VHOST_ACCESS_RO);
+		if (unlikely(src == NULL || len == 0))
+			return 0;
 
-	rte_memcpy((uint8_t *)data, src, dlen);
-	data += dlen;
+		rte_memcpy(dst, src, len);
+		remain -= len;
+		/* cast is needed for 32-bit architecture */
+		dst = RTE_PTR_ADD(dst, (size_t)len);
+		addr += len;
+	} while (unlikely(remain != 0));
 
-	if (unlikely(dlen < to_copy)) {
-		remain = to_copy - dlen;
-		addr = desc->addr + dlen;
+	return RTE_MIN(desc->len, size);
+}
 
-		while (remain) {
-			len = remain;
-			src = IOVA_TO_VVA(uint8_t *, vc_req, addr, &len,
-					VHOST_ACCESS_RO);
-			if (unlikely(!src || !len)) {
-				VC_LOG_ERR("Failed to map descriptor");
-				return -1;
-			}
 
-			rte_memcpy(data, src, len);
-			addr += len;
-			remain -= len;
-			data += len;
-		}
-	}
+static __rte_always_inline int
+copy_data(void *data, struct vhost_crypto_data_req *vc_req,
+	struct vhost_crypto_desc *head, struct vhost_crypto_desc **cur_desc,
+	uint32_t size, uint32_t max_n_descs)
+{
+	struct vhost_crypto_desc *desc = *cur_desc;
+	uint32_t left = size;
 
-	left -= to_copy;
+	do {
+		uint32_t copied;
 
-	while (desc >= head && desc - head < (int)max_n_descs && left) {
-		desc++;
-		to_copy = RTE_MIN(desc->len, (uint32_t)left);
-		dlen = to_copy;
-		src = IOVA_TO_VVA(uint8_t *, vc_req, desc->addr, &dlen,
-				VHOST_ACCESS_RO);
-		if (unlikely(!src || !dlen)) {
-			VC_LOG_ERR("Failed to map descriptor");
+		copied = copy_data_from_desc(data, vc_req, desc, left);
+		if (copied == 0)
 			return -1;
-		}
-
-		rte_memcpy(data, src, dlen);
-		data += dlen;
-
-		if (unlikely(dlen < to_copy)) {
-			remain = to_copy - dlen;
-			addr = desc->addr + dlen;
-
-			while (remain) {
-				len = remain;
-				src = IOVA_TO_VVA(uint8_t *, vc_req, addr, &len,
-						VHOST_ACCESS_RO);
-				if (unlikely(!src || !len)) {
-					VC_LOG_ERR("Failed to map descriptor");
-					return -1;
-				}
-
-				rte_memcpy(data, src, len);
-				addr += len;
-				remain -= len;
-				data += len;
-			}
-		}
-
-		left -= to_copy;
-	}
+		left -= copied;
+		data = RTE_PTR_ADD(data, copied);
+		desc++;
+	} while (desc < head + max_n_descs && left != 0);
 
-	if (unlikely(left > 0)) {
-		VC_LOG_ERR("Incorrect virtio descriptor");
+	if (unlikely(left != 0))
 		return -1;
-	}
 
-	if (unlikely(desc - head == (int)max_n_descs))
+	if (unlikely(desc == head + max_n_descs))
 		*cur_desc = NULL;
 	else
 		*cur_desc = desc + 1;
@@ -852,6 +816,7 @@ prepare_sym_cipher_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 	/* iv */
 	if (unlikely(copy_data(iv_data, vc_req, head, &desc,
 			cipher->para.iv_len, max_n_descs))) {
+		VC_LOG_ERR("Incorrect virtio descriptor");
 		ret = VIRTIO_CRYPTO_BADMSG;
 		goto error_exit;
 	}
@@ -883,6 +848,7 @@ prepare_sym_cipher_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 		if (unlikely(copy_data(rte_pktmbuf_mtod(m_src, uint8_t *),
 				vc_req, head, &desc, cipher->para.src_data_len,
 				max_n_descs) < 0)) {
+			VC_LOG_ERR("Incorrect virtio descriptor");
 			ret = VIRTIO_CRYPTO_BADMSG;
 			goto error_exit;
 		}
@@ -1006,6 +972,7 @@ prepare_sym_chain_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 	/* iv */
 	if (unlikely(copy_data(iv_data, vc_req, head, &desc,
 			chain->para.iv_len, max_n_descs) < 0)) {
+		VC_LOG_ERR("Incorrect virtio descriptor");
 		ret = VIRTIO_CRYPTO_BADMSG;
 		goto error_exit;
 	}
@@ -1037,6 +1004,7 @@ prepare_sym_chain_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 		if (unlikely(copy_data(rte_pktmbuf_mtod(m_src, uint8_t *),
 				vc_req, head, &desc, chain->para.src_data_len,
 				max_n_descs) < 0)) {
+			VC_LOG_ERR("Incorrect virtio descriptor");
 			ret = VIRTIO_CRYPTO_BADMSG;
 			goto error_exit;
 		}
@@ -1121,6 +1089,7 @@ prepare_sym_chain_op(struct vhost_crypto *vcrypto, struct rte_crypto_op *op,
 		if (unlikely(copy_data(digest_addr, vc_req, head, &digest_desc,
 				chain->para.hash_result_len,
 				max_n_descs) < 0)) {
+			VC_LOG_ERR("Incorrect virtio descriptor");
 			ret = VIRTIO_CRYPTO_BADMSG;
 			goto error_exit;
 		}
-- 
2.36.1

Re: [PATCH v3] vhost/crypto: fix build with GCC 12

[Maxime Coquelin]

On 6/16/22 16:46, David Marchand wrote:
> GCC 12 raises the following warning:
> 
> In file included from ../lib/mempool/rte_mempool.h:46,
>                   from ../lib/mbuf/rte_mbuf.h:38,
>                   from ../lib/vhost/vhost_crypto.c:7:
> ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
>       outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
>       [-Warray-bounds]
>    371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
>   1178 |         struct virtio_crypto_op_data_req req;
>        |                                          ^~~
> 
> Split this function and separate the per descriptor copy.
> This makes the code clearer, and the compiler happier.
> 
> Note: logs for errors have been moved to callers to avoid duplicates.
> 
> Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v2:
> - fixed 32-bits build,
> 
> Changes since v1:
> - refactored copy function,
> 
> ---
>   lib/vhost/vhost_crypto.c | 123 +++++++++++++++------------------------
>   1 file changed, 46 insertions(+), 77 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I'll wait until tuesday to apply it to my tree to let some time for testing

Thanks,
Maxime

Re: [PATCH v3] vhost/crypto: fix build with GCC 12

[Maxime Coquelin]

On 6/16/22 16:46, David Marchand wrote:
> GCC 12 raises the following warning:
> 
> In file included from ../lib/mempool/rte_mempool.h:46,
>                   from ../lib/mbuf/rte_mbuf.h:38,
>                   from ../lib/vhost/vhost_crypto.c:7:
> ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
>       outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
>       [-Warray-bounds]
>    371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
>   1178 |         struct virtio_crypto_op_data_req req;
>        |                                          ^~~
> 
> Split this function and separate the per descriptor copy.
> This makes the code clearer, and the compiler happier.
> 
> Note: logs for errors have been moved to callers to avoid duplicates.
> 
> Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v2:
> - fixed 32-bits build,
> 
> Changes since v1:
> - refactored copy function,
> 
> ---
>   lib/vhost/vhost_crypto.c | 123 +++++++++++++++------------------------
>   1 file changed, 46 insertions(+), 77 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime

RE: [PATCH v3] vhost/crypto: fix build with GCC 12

[Poczatek, Jakub]

Hey everyone, 

When running a Virtio performance test on a VM using VHost with this patch applied,
VHost gives the following error message: 

> VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_FEATURES
> VHOST_CONFIG: (/tmp/vhost) negotiated Virtio features: 0x0
> VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_MEM_TABLE
> VHOST_CONFIG: (/tmp/vhost) guest memory region size: 0x80000000
> VHOST_CONFIG: (/tmp/vhost)       guest physical addr: 0x0
> VHOST_CONFIG: (/tmp/vhost)       guest virtual  addr: 0x7f17c0000000
> VHOST_CONFIG: (/tmp/vhost)       host  virtual  addr: 0x7f94c0000000
> VHOST_CONFIG: (/tmp/vhost)       mmap addr : 0x7f94c0000000
> VHOST_CONFIG: (/tmp/vhost)       mmap size : 0x80000000
> VHOST_CONFIG: (/tmp/vhost)       mmap align: 0x40000000
> VHOST_CONFIG: (/tmp/vhost)       mmap off  : 0x0
> VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_NUM
> VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_BASE
> VHOST_CONFIG: (/tmp/vhost) vring base idx:0 last_used_idx:0 last_avail_idx:0.
> VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_ADDR
> VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_KICK
> VHOST_CONFIG: (/tmp/vhost) vring kick idx:0 file:37
> VHOST_CONFIG: (/tmp/vhost) reallocated virtqueue on node 1
> VHOST_CONFIG: (/tmp/vhost) reallocated device on node 1
> VHOST_CONFIG: (/tmp/vhost) virtio is now ready for processing.
> USER1: New Vhost-crypto Device /tmp/vhost, Device ID 0
> VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: (/tmp/vhost) vring call idx:0 file:38
> USER1: [VHOST-Crypto]: Session 1 created for vdev 0.
> USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> USER1: [VHOST-Crypto]: Failed to process sym request
> USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> USER1: [VHOST-Crypto]: Failed to process sym request

Due to this, performance test hangs and never finishes. 

Kind Regards, 
Jakub Poczatek

-----Original Message-----
From: Maxime Coquelin <maxime.coquelin@redhat.com> 
Sent: Tuesday 21 June 2022 10:31
To: David Marchand <david.marchand@redhat.com>; dev@dpdk.org
Cc: stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>
Subject: Re: [PATCH v3] vhost/crypto: fix build with GCC 12



On 6/16/22 16:46, David Marchand wrote:
> GCC 12 raises the following warning:
> 
> In file included from ../lib/mempool/rte_mempool.h:46,
>                   from ../lib/mbuf/rte_mbuf.h:38,
>                   from ../lib/vhost/vhost_crypto.c:7:
> ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
>       outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
>       [-Warray-bounds]
>    371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
>   1178 |         struct virtio_crypto_op_data_req req;
>        |                                          ^~~
> 
> Split this function and separate the per descriptor copy.
> This makes the code clearer, and the compiler happier.
> 
> Note: logs for errors have been moved to callers to avoid duplicates.
> 
> Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v2:
> - fixed 32-bits build,
> 
> Changes since v1:
> - refactored copy function,
> 
> ---
>   lib/vhost/vhost_crypto.c | 123 +++++++++++++++------------------------
>   1 file changed, 46 insertions(+), 77 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime

RE: [PATCH v3] vhost/crypto: fix build with GCC 12

[Zhang, Roy Fan]

Hi Maxime,

I know it is over Tuesday so we understand you merged the patch already.
But any suggestions? Should we raise a Bugzilla for this problem?
BTW we reverted the patch and the test finished no problem.

Regards,
Fan
> -----Original Message-----
> From: Poczatek, Jakub <jakub.poczatek@intel.com>
> Sent: Wednesday, June 22, 2022 10:02 AM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; David Marchand
> <david.marchand@redhat.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>
> Subject: RE: [PATCH v3] vhost/crypto: fix build with GCC 12
> 
> Hey everyone,
> 
> When running a Virtio performance test on a VM using VHost with this patch
> applied,
> VHost gives the following error message:
> 
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_FEATURES
> > VHOST_CONFIG: (/tmp/vhost) negotiated Virtio features: 0x0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_MEM_TABLE
> > VHOST_CONFIG: (/tmp/vhost) guest memory region size: 0x80000000
> > VHOST_CONFIG: (/tmp/vhost)       guest physical addr: 0x0
> > VHOST_CONFIG: (/tmp/vhost)       guest virtual  addr: 0x7f17c0000000
> > VHOST_CONFIG: (/tmp/vhost)       host  virtual  addr: 0x7f94c0000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap addr : 0x7f94c0000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap size : 0x80000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap align: 0x40000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap off  : 0x0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_NUM
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_BASE
> > VHOST_CONFIG: (/tmp/vhost) vring base idx:0 last_used_idx:0
> last_avail_idx:0.
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_ADDR
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_KICK
> > VHOST_CONFIG: (/tmp/vhost) vring kick idx:0 file:37
> > VHOST_CONFIG: (/tmp/vhost) reallocated virtqueue on node 1
> > VHOST_CONFIG: (/tmp/vhost) reallocated device on node 1
> > VHOST_CONFIG: (/tmp/vhost) virtio is now ready for processing.
> > USER1: New Vhost-crypto Device /tmp/vhost, Device ID 0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: (/tmp/vhost) vring call idx:0 file:38
> > USER1: [VHOST-Crypto]: Session 1 created for vdev 0.
> > USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> > USER1: [VHOST-Crypto]: Failed to process sym request
> > USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> > USER1: [VHOST-Crypto]: Failed to process sym request
> 
> Due to this, performance test hangs and never finishes.
> 
> Kind Regards,
> Jakub Poczatek
> 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday 21 June 2022 10:31
> To: David Marchand <david.marchand@redhat.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>
> Subject: Re: [PATCH v3] vhost/crypto: fix build with GCC 12
> 
> 
> 
> On 6/16/22 16:46, David Marchand wrote:
> > GCC 12 raises the following warning:
> >
> > In file included from ../lib/mempool/rte_mempool.h:46,
> >                   from ../lib/mbuf/rte_mbuf.h:38,
> >                   from ../lib/vhost/vhost_crypto.c:7:
> > ../lib/vhost/vhost_crypto.c: In function ‘rte_vhost_crypto_fetch_requests’:
> > ../lib/eal/x86/include/rte_memcpy.h:371:9: warning: array subscript 1 is
> >       outside array bounds of ‘struct virtio_crypto_op_data_req[1]’
> >       [-Warray-bounds]
> >    371 | rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
> >        |
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ../lib/vhost/vhost_crypto.c:1178:42: note: while referencing ‘req’
> >   1178 |         struct virtio_crypto_op_data_req req;
> >        |                                          ^~~
> >
> > Split this function and separate the per descriptor copy.
> > This makes the code clearer, and the compiler happier.
> >
> > Note: logs for errors have been moved to callers to avoid duplicates.
> >
> > Fixes: 3c79609fda7c ("vhost/crypto: handle virtually non-contiguous buffers")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > Changes since v2:
> > - fixed 32-bits build,
> >
> > Changes since v1:
> > - refactored copy function,
> >
> > ---
> >   lib/vhost/vhost_crypto.c | 123 +++++++++++++++------------------------
> >   1 file changed, 46 insertions(+), 77 deletions(-)
> >
> 
> Applied to dpdk-next-virtio/main.
> 
> Thanks,
> Maxime

Re: [PATCH v3] vhost/crypto: fix build with GCC 12

[David Marchand]

Hello Jakub, Roy,

On Wed, Jun 22, 2022 at 11:01 AM Poczatek, Jakub
<jakub.poczatek@intel.com> wrote:
> When running a Virtio performance test on a VM using VHost with this patch applied,
> VHost gives the following error message:
>
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_FEATURES
> > VHOST_CONFIG: (/tmp/vhost) negotiated Virtio features: 0x0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_MEM_TABLE
> > VHOST_CONFIG: (/tmp/vhost) guest memory region size: 0x80000000
> > VHOST_CONFIG: (/tmp/vhost)       guest physical addr: 0x0
> > VHOST_CONFIG: (/tmp/vhost)       guest virtual  addr: 0x7f17c0000000
> > VHOST_CONFIG: (/tmp/vhost)       host  virtual  addr: 0x7f94c0000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap addr : 0x7f94c0000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap size : 0x80000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap align: 0x40000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap off  : 0x0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_NUM
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_BASE
> > VHOST_CONFIG: (/tmp/vhost) vring base idx:0 last_used_idx:0 last_avail_idx:0.
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_ADDR
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_KICK
> > VHOST_CONFIG: (/tmp/vhost) vring kick idx:0 file:37
> > VHOST_CONFIG: (/tmp/vhost) reallocated virtqueue on node 1
> > VHOST_CONFIG: (/tmp/vhost) reallocated device on node 1
> > VHOST_CONFIG: (/tmp/vhost) virtio is now ready for processing.
> > USER1: New Vhost-crypto Device /tmp/vhost, Device ID 0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: (/tmp/vhost) vring call idx:0 file:38
> > USER1: [VHOST-Crypto]: Session 1 created for vdev 0.
> > USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> > USER1: [VHOST-Crypto]: Failed to process sym request
> > USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> > USER1: [VHOST-Crypto]: Failed to process sym request

Could you test with the following snippet:

diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c
index 96ffb82a5d..54946f46d9 100644
--- a/lib/vhost/vhost_crypto.c
+++ b/lib/vhost/vhost_crypto.c
@@ -610,8 +610,7 @@ copy_data(void *data, struct vhost_crypto_data_req *vc_req,
                        return -1;
                left -= copied;
                data = RTE_PTR_ADD(data, copied);
-               desc++;
-       } while (desc < head + max_n_descs && left != 0);
+       } while (left != 0 && ++desc < head + max_n_descs);

        if (unlikely(left != 0))
                return -1;


-- 
David Marchand

RE: [PATCH v3] vhost/crypto: fix build with GCC 12

[Poczatek, Jakub]

Hey David, 

The code change fixes the errors and the performance test completes. 

Kind Regards, 
Jakub Poczatek

-----Original Message-----
From: David Marchand <david.marchand@redhat.com> 
Sent: Wednesday 22 June 2022 15:08
To: Poczatek, Jakub <jakub.poczatek@intel.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org; stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
Subject: Re: [PATCH v3] vhost/crypto: fix build with GCC 12

Hello Jakub, Roy,

On Wed, Jun 22, 2022 at 11:01 AM Poczatek, Jakub <jakub.poczatek@intel.com> wrote:
> When running a Virtio performance test on a VM using VHost with this 
> patch applied, VHost gives the following error message:
>
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_FEATURES
> > VHOST_CONFIG: (/tmp/vhost) negotiated Virtio features: 0x0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_MEM_TABLE
> > VHOST_CONFIG: (/tmp/vhost) guest memory region size: 0x80000000
> > VHOST_CONFIG: (/tmp/vhost)       guest physical addr: 0x0
> > VHOST_CONFIG: (/tmp/vhost)       guest virtual  addr: 0x7f17c0000000
> > VHOST_CONFIG: (/tmp/vhost)       host  virtual  addr: 0x7f94c0000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap addr : 0x7f94c0000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap size : 0x80000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap align: 0x40000000
> > VHOST_CONFIG: (/tmp/vhost)       mmap off  : 0x0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_NUM
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_BASE
> > VHOST_CONFIG: (/tmp/vhost) vring base idx:0 last_used_idx:0 last_avail_idx:0.
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_ADDR
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_KICK
> > VHOST_CONFIG: (/tmp/vhost) vring kick idx:0 file:37
> > VHOST_CONFIG: (/tmp/vhost) reallocated virtqueue on node 1
> > VHOST_CONFIG: (/tmp/vhost) reallocated device on node 1
> > VHOST_CONFIG: (/tmp/vhost) virtio is now ready for processing.
> > USER1: New Vhost-crypto Device /tmp/vhost, Device ID 0
> > VHOST_CONFIG: (/tmp/vhost) read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: (/tmp/vhost) vring call idx:0 file:38
> > USER1: [VHOST-Crypto]: Session 1 created for vdev 0.
> > USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> > USER1: [VHOST-Crypto]: Failed to process sym request
> > USER1: [VHOST-Crypto]: Incorrect virtio descriptor
> > USER1: [VHOST-Crypto]: Failed to process sym request

Could you test with the following snippet:

diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c index 96ffb82a5d..54946f46d9 100644
--- a/lib/vhost/vhost_crypto.c
+++ b/lib/vhost/vhost_crypto.c
@@ -610,8 +610,7 @@ copy_data(void *data, struct vhost_crypto_data_req *vc_req,
                        return -1;
                left -= copied;
                data = RTE_PTR_ADD(data, copied);
-               desc++;
-       } while (desc < head + max_n_descs && left != 0);
+       } while (left != 0 && ++desc < head + max_n_descs);

        if (unlikely(left != 0))
                return -1;


--
David Marchand

Re: [PATCH v3] vhost/crypto: fix build with GCC 12

[David Marchand]

On Wed, Jun 22, 2022 at 5:22 PM Poczatek, Jakub
<jakub.poczatek@intel.com> wrote:
>
> Hey David,
>
> The code change fixes the errors and the performance test completes.

I posted this fix against (not yet pulled into main) next-virtio repo.

-- 
David Marchand