diff mbox series

[v4,10/10] common/dpaxx: remove zero length array

Message ID 20231120170942.197172-11-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Replace zero length arrays |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Stephen Hemminger Nov. 20, 2023, 5:07 p.m. UTC 
  There is a place holder zero length array in this driver.
But since the structure is embedded in other structures,
it could not have been safely used anyway.
There doesn't appear to be any uses of it in the current code.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/common/dpaax/caamflib/desc/ipsec.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Hemant Agrawal Nov. 21, 2023, 10:49 a.m. UTC | #1 
> There is a place holder zero length array in this driver.
> But since the structure is embedded in other structures,
> it could not have been safely used anyway.
> There doesn't appear to be any uses of it in the current code.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/common/dpaax/caamflib/desc/ipsec.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/common/dpaax/caamflib/desc/ipsec.h
> b/drivers/common/dpaax/caamflib/desc/ipsec.h
> index 95fc3ea5ba3b..9d59b93292f9 100644
> --- a/drivers/common/dpaax/caamflib/desc/ipsec.h
> +++ b/drivers/common/dpaax/caamflib/desc/ipsec.h
> @@ -336,7 +336,6 @@ struct ipsec_encap_gcm {
>   * @ip_hdr_len: optional IP Header length (in bytes)
>   *  reserved - 16b
>   *  Opt. IP Hdr Len - 16b
> - * @ip_hdr: optional IP Header content (only for IPsec legacy mode)
>   */
>  struct ipsec_encap_pdb {
>  	uint32_t options;
> @@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
>  	};
>  	uint32_t spi;
>  	uint32_t ip_hdr_len;
> -	uint8_t ip_hdr[0];
[Hemant] This should be replaced with 
	uint8_t ip_hdr[];
>  };
> 
>  static inline unsigned int
> @@ -776,7 +774,7 @@ cnstr_shdsc_ipsec_encap(uint32_t *descbuf, bool ps,
> bool swap,
>  		PROGRAM_SET_36BIT_ADDR(p);
>  	phdr = SHR_HDR(p, share, hdr, 0);
>  	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
> -	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
> +
[Hemant] Don't remove it. It will break the code.

>  	SET_LABEL(p, hdr);
>  	pkeyjmp = JUMP(p, keyjmp, LOCAL_JUMP, ALL_TRUE, BOTH|SHRD);
>  	if (authdata->keylen)
> @@ -913,7 +911,7 @@ cnstr_shdsc_ipsec_encap_des_aes_xcbc(uint32_t
> *descbuf,
>  	PROGRAM_CNTXT_INIT(p, descbuf, 0);
>  	phdr = SHR_HDR(p, share, hdr, 0);
>  	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
> -	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
> +
[Hemant] Don't remove it

>  	SET_LABEL(p, hdr);
>  	pkeyjump = JUMP(p, keyjump, LOCAL_JUMP, ALL_TRUE, SHRD |
> SELF);
>  	/*
> --
> 2.42.0
Stephen Hemminger Nov. 21, 2023, 4:46 p.m. UTC | #2 
On Tue, 21 Nov 2023 10:49:59 +0000
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> >  struct ipsec_encap_pdb {
> >  	uint32_t options;
> > @@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
> >  	};
> >  	uint32_t spi;
> >  	uint32_t ip_hdr_len;
> > -	uint8_t ip_hdr[0];  
> [Hemant] This should be replaced with 
> 	uint8_t ip_hdr[];
> >  };
> > 

That won't work because the structure is embedded in
another struct and then clang will correctly report an error.
Stephen Hemminger Nov. 21, 2023, 5:01 p.m. UTC | #3 
On Tue, 21 Nov 2023 08:46:41 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Tue, 21 Nov 2023 10:49:59 +0000
> Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> 
> > >  struct ipsec_encap_pdb {
> > >  	uint32_t options;
> > > @@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
> > >  	};
> > >  	uint32_t spi;
> > >  	uint32_t ip_hdr_len;
> > > -	uint8_t ip_hdr[0];    
> > [Hemant] This should be replaced with 
> > 	uint8_t ip_hdr[];  
> > >  };
> > >   
> 
> That won't work because the structure is embedded in
> another struct and then clang will correctly report an error.

[2155/2868] Compiling C object drivers..._jr.a.p/crypto_caam_jr_caam_jr_uio.c.o
In file included from ../drivers/crypto/caam_jr/caam_jr_uio.c:23:
../drivers/crypto/caam_jr/caam_jr_pvt.h:139:25: warning: field 'encap_pdb' with variable sized type 'struct ipsec_encap_pdb' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct ipsec_encap_pdb encap_pdb;
                               ^
1 warning generated.
[2160/2868] Compiling C object drivers...m_jr.a.p/crypto_caam_jr_caam_jr_hw.c.o
In file included from ../drivers/crypto/caam_jr/caam_jr_hw.c:16:
../drivers/crypto/caam_jr/caam_jr_pvt.h:139:25: warning: field 'encap_pdb' with variable sized type 'struct ipsec_encap_pdb' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct ipsec_encap_pdb encap_pdb;
                               ^
1 warning generated.
[2174/2868] Compiling C object drivers....p/crypto_dpaa_sec_dpaa_sec_raw_dp.c.o
In file included from ../drivers/crypto/dpaa_sec/dpaa_sec_raw_dp.c:17:
../drivers/crypto/dpaa_sec/dpaa_sec.h:190:27: warning: field 'encap_pdb' with variable sized type 'struct ipsec_encap_pdb' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
                        struct ipsec_encap_pdb encap_pdb;
                                               ^
1 warning generated.
[2176/2868] Compiling C object drivers...caam_jr.a.p/crypto_caam_jr_caam_jr.c.o
In file included from ../drivers/crypto/caam_jr/caam_jr.c:23:
../drivers/crypto/caam_jr/caam_jr_pvt.h:139:25: warning: field 'encap_pdb' with variable sized type 'struct ipsec_encap_pdb' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct ipsec_encap_pdb encap_pdb;
                               ^
1 warning generated.
[2246/2868] Compiling C object drivers...a_sec.a.p/crypto_dpaa_sec_dpaa_sec.c.o
In file included from ../drivers/crypto/dpaa_sec/dpaa_sec.c:43:
../drivers/crypto/dpaa_sec/dpaa_sec.h:190:27: warning: field 'encap_pdb' with variable sized type 'struct ipsec_encap_pdb' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
                        struct ipsec_encap_pdb encap_pdb;
                                               ^
1 warning generated.
Tyler Retzlaff Nov. 21, 2023, 5:18 p.m. UTC | #4 
On Tue, Nov 21, 2023 at 08:46:41AM -0800, Stephen Hemminger wrote:
> On Tue, 21 Nov 2023 10:49:59 +0000
> Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> 
> > >  struct ipsec_encap_pdb {
> > >  	uint32_t options;
> > > @@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
> > >  	};
> > >  	uint32_t spi;
> > >  	uint32_t ip_hdr_len;
> > > -	uint8_t ip_hdr[0];  
> > [Hemant] This should be replaced with 
> > 	uint8_t ip_hdr[];
> > >  };
> > > 
> 
> That won't work because the structure is embedded in
> another struct and then clang will correctly report an error.

Hemant if the field is not referenced what's the harm in removing it?
Hemant Agrawal Nov. 22, 2023, 7:23 a.m. UTC | #5 
> -----Original Message-----
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Sent: Tuesday, November 21, 2023 10:49 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; dev@dpdk.org; Sachin
> Saxena <sachin.saxena@nxp.com>; Gagandeep Singh <G.Singh@nxp.com>
> Subject: Re: [PATCH v4 10/10] common/dpaxx: remove zero length array
> Importance: High
> 
> On Tue, Nov 21, 2023 at 08:46:41AM -0800, Stephen Hemminger wrote:
> > On Tue, 21 Nov 2023 10:49:59 +0000
> > Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> >
> > > >  struct ipsec_encap_pdb {
> > > >  	uint32_t options;
> > > > @@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
> > > >  	};
> > > >  	uint32_t spi;
> > > >  	uint32_t ip_hdr_len;
> > > > -	uint8_t ip_hdr[0];
> > > [Hemant] This should be replaced with
> > > 	uint8_t ip_hdr[];
> > > >  };
> > > >
> >
> > That won't work because the structure is embedded in another struct
> > and then clang will correctly report an error.
> 
> Hemant if the field is not referenced what's the harm in removing it?

It is being used indirectly, e.g. dpaa_sec.h usages it for ip address description preparation for the hardware.

            struct ipsec_encap_pdb encap_pdb;
            union {
                struct ip ip4_hdr;
                struct rte_ipv6_hdr ip6_hdr;
            };

We need to change it, however in the interim, I will like to add "-Wno-gnu-variable-sized-type-not-at-end" for both caam_jr and dpaa_sec to avoid this warning.
Stephen Hemminger Nov. 23, 2023, 1:43 a.m. UTC | #6 
On Wed, 22 Nov 2023 07:23:36 +0000
Hemant Agrawal <hemant.agrawal@nxp.com> wrote:

> > -----Original Message-----
> > From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > Sent: Tuesday, November 21, 2023 10:49 PM
> > To: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Hemant Agrawal <hemant.agrawal@nxp.com>; dev@dpdk.org; Sachin
> > Saxena <sachin.saxena@nxp.com>; Gagandeep Singh <G.Singh@nxp.com>
> > Subject: Re: [PATCH v4 10/10] common/dpaxx: remove zero length array
> > Importance: High
> > 
> > On Tue, Nov 21, 2023 at 08:46:41AM -0800, Stephen Hemminger wrote:  
> > > On Tue, 21 Nov 2023 10:49:59 +0000
> > > Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> > >  
> > > > >  struct ipsec_encap_pdb {
> > > > >  	uint32_t options;
> > > > > @@ -350,7 +349,6 @@ struct ipsec_encap_pdb {
> > > > >  	};
> > > > >  	uint32_t spi;
> > > > >  	uint32_t ip_hdr_len;
> > > > > -	uint8_t ip_hdr[0];  
> > > > [Hemant] This should be replaced with
> > > > 	uint8_t ip_hdr[];  
> > > > >  };
> > > > >  
> > >
> > > That won't work because the structure is embedded in another struct
> > > and then clang will correctly report an error.  
> > 
> > Hemant if the field is not referenced what's the harm in removing it?  
> 
> It is being used indirectly, e.g. dpaa_sec.h usages it for ip address description preparation for the hardware.
> 
>             struct ipsec_encap_pdb encap_pdb;
>             union {
>                 struct ip ip4_hdr;
>                 struct rte_ipv6_hdr ip6_hdr;
>             };
> 
> We need to change it, however in the interim, I will like to add "-Wno-gnu-variable-sized-type-not-at-end" for both caam_jr and dpaa_sec to avoid this warning.

Ok, there are several ways to fix that. Driver specific compiler flags are the one
that is least favored.
Stephen Hemminger Nov. 23, 2023, 7:18 p.m. UTC | #7 
This is a better alternative.

From 52c805b9526dbef62377276c4499c997fbc96268 Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu, 23 Nov 2023 11:12:02 -0800
Subject: [PATCH] common/dpaxx: replace zero length array

The zero length ip_header is used as an overlay to the
encap IP header. Since the code is already assuming the layout
of the structure, replace the array with direct access.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/common/dpaax/caamflib/desc/ipsec.h | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/common/dpaax/caamflib/desc/ipsec.h b/drivers/common/dpaax/caamflib/desc/ipsec.h
index 95fc3ea5ba3b..d1411cc6aab4 100644
--- a/drivers/common/dpaax/caamflib/desc/ipsec.h
+++ b/drivers/common/dpaax/caamflib/desc/ipsec.h
@@ -334,9 +334,7 @@ struct ipsec_encap_gcm {
  * @seq_num: IPsec sequence number
  * @spi: IPsec SPI (Security Parameters Index)
  * @ip_hdr_len: optional IP Header length (in bytes)
- *  reserved - 16b
- *  Opt. IP Hdr Len - 16b
- * @ip_hdr: optional IP Header content (only for IPsec legacy mode)
+ *  Ip header must follow directly after ipsec_encap_pdb
  */
 struct ipsec_encap_pdb {
 	uint32_t options;
@@ -350,7 +348,6 @@ struct ipsec_encap_pdb {
 	};
 	uint32_t spi;
 	uint32_t ip_hdr_len;
-	uint8_t ip_hdr[0];
 };
 
 static inline unsigned int
@@ -776,7 +773,12 @@ cnstr_shdsc_ipsec_encap(uint32_t *descbuf, bool ps, bool swap,
 		PROGRAM_SET_36BIT_ADDR(p);
 	phdr = SHR_HDR(p, share, hdr, 0);
 	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
+	/* ip header if any follows the encap_pdb */
+	if (pdb->ip_hdr_len > 0) {
+		void *ip_hdr = pdb + 1;
+		COPY_DATA(p, ip_hdr, pdb->ip_hdr_len);
+	}
 	SET_LABEL(p, hdr);
 	pkeyjmp = JUMP(p, keyjmp, LOCAL_JUMP, ALL_TRUE, BOTH|SHRD);
 	if (authdata->keylen)
@@ -913,7 +915,13 @@ cnstr_shdsc_ipsec_encap_des_aes_xcbc(uint32_t *descbuf,
 	PROGRAM_CNTXT_INIT(p, descbuf, 0);
 	phdr = SHR_HDR(p, share, hdr, 0);
 	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
+	/* ip header if any follows the encap_pdb */
+	if (pdb->ip_hdr_len > 0) {
+		void *ip_hdr = pdb + 1;
+		COPY_DATA(p, ip_hdr, pdb->ip_hdr_len);
+	}
+
 	SET_LABEL(p, hdr);
 	pkeyjump = JUMP(p, keyjump, LOCAL_JUMP, ALL_TRUE, SHRD | SELF);
 	/*
Hemant Agrawal Nov. 28, 2023, 6:43 a.m. UTC | #8 
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, November 24, 2023 12:49 AM
> To: Hemant Agrawal <hemant.agrawal@nxp.com>
> Cc: Tyler Retzlaff <roretzla@linux.microsoft.com>; dev@dpdk.org; Sachin
> Saxena <sachin.saxena@nxp.com>; Gagandeep Singh <G.Singh@nxp.com>
> Subject: Re: [PATCH v4 10/10] common/dpaxx: remove zero length array
> Importance: High
> 
> This is a better alternative.
> 
> From 52c805b9526dbef62377276c4499c997fbc96268 Mon Sep 17 00:00:00
> 2001
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 23 Nov 2023 11:12:02 -0800
> Subject: [PATCH] common/dpaxx: replace zero length array
> 
> The zero length ip_header is used as an overlay to the encap IP header. Since
> the code is already assuming the layout of the structure, replace the array
> with direct access.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/common/dpaax/caamflib/desc/ipsec.h | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/common/dpaax/caamflib/desc/ipsec.h
> b/drivers/common/dpaax/caamflib/desc/ipsec.h
> index 95fc3ea5ba3b..d1411cc6aab4 100644
> --- a/drivers/common/dpaax/caamflib/desc/ipsec.h
> +++ b/drivers/common/dpaax/caamflib/desc/ipsec.h
> @@ -334,9 +334,7 @@ struct ipsec_encap_gcm {
>   * @seq_num: IPsec sequence number
>   * @spi: IPsec SPI (Security Parameters Index)
>   * @ip_hdr_len: optional IP Header length (in bytes)
> - *  reserved - 16b
> - *  Opt. IP Hdr Len - 16b
> - * @ip_hdr: optional IP Header content (only for IPsec legacy mode)
> + *  Ip header must follow directly after ipsec_encap_pdb
>   */
>  struct ipsec_encap_pdb {
>  	uint32_t options;
> @@ -350,7 +348,6 @@ struct ipsec_encap_pdb {
>  	};
>  	uint32_t spi;
>  	uint32_t ip_hdr_len;
> -	uint8_t ip_hdr[0];
>  };
> 
>  static inline unsigned int
> @@ -776,7 +773,12 @@ cnstr_shdsc_ipsec_encap(uint32_t *descbuf, bool
> ps, bool swap,
>  		PROGRAM_SET_36BIT_ADDR(p);
>  	phdr = SHR_HDR(p, share, hdr, 0);
>  	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
> -	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
> +
> +	/* ip header if any follows the encap_pdb */
> +	if (pdb->ip_hdr_len > 0) {
> +		void *ip_hdr = pdb + 1;
> +		COPY_DATA(p, ip_hdr, pdb->ip_hdr_len);
> +	}
>  	SET_LABEL(p, hdr);
>  	pkeyjmp = JUMP(p, keyjmp, LOCAL_JUMP, ALL_TRUE, BOTH|SHRD);
>  	if (authdata->keylen)
> @@ -913,7 +915,13 @@ cnstr_shdsc_ipsec_encap_des_aes_xcbc(uint32_t
> *descbuf,
>  	PROGRAM_CNTXT_INIT(p, descbuf, 0);
>  	phdr = SHR_HDR(p, share, hdr, 0);
>  	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
> -	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
> +
> +	/* ip header if any follows the encap_pdb */
> +	if (pdb->ip_hdr_len > 0) {
> +		void *ip_hdr = pdb + 1;
> +		COPY_DATA(p, ip_hdr, pdb->ip_hdr_len);
> +	}
> +
>  	SET_LABEL(p, hdr);
>  	pkeyjump = JUMP(p, keyjump, LOCAL_JUMP, ALL_TRUE, SHRD |
> SELF);
>  	/*
> --
> 2.42.0
diff mbox series

Patch

diff --git a/drivers/common/dpaax/caamflib/desc/ipsec.h b/drivers/common/dpaax/caamflib/desc/ipsec.h
index 95fc3ea5ba3b..9d59b93292f9 100644
--- a/drivers/common/dpaax/caamflib/desc/ipsec.h
+++ b/drivers/common/dpaax/caamflib/desc/ipsec.h
@@ -336,7 +336,6 @@  struct ipsec_encap_gcm {
  * @ip_hdr_len: optional IP Header length (in bytes)
  *  reserved - 16b
  *  Opt. IP Hdr Len - 16b
- * @ip_hdr: optional IP Header content (only for IPsec legacy mode)
  */
 struct ipsec_encap_pdb {
 	uint32_t options;
@@ -350,7 +349,6 @@  struct ipsec_encap_pdb {
 	};
 	uint32_t spi;
 	uint32_t ip_hdr_len;
-	uint8_t ip_hdr[0];
 };
 
 static inline unsigned int
@@ -776,7 +774,7 @@  cnstr_shdsc_ipsec_encap(uint32_t *descbuf, bool ps, bool swap,
 		PROGRAM_SET_36BIT_ADDR(p);
 	phdr = SHR_HDR(p, share, hdr, 0);
 	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
 	SET_LABEL(p, hdr);
 	pkeyjmp = JUMP(p, keyjmp, LOCAL_JUMP, ALL_TRUE, BOTH|SHRD);
 	if (authdata->keylen)
@@ -913,7 +911,7 @@  cnstr_shdsc_ipsec_encap_des_aes_xcbc(uint32_t *descbuf,
 	PROGRAM_CNTXT_INIT(p, descbuf, 0);
 	phdr = SHR_HDR(p, share, hdr, 0);
 	__rta_copy_ipsec_encap_pdb(p, pdb, cipherdata->algtype);
-	COPY_DATA(p, pdb->ip_hdr, pdb->ip_hdr_len);
+
 	SET_LABEL(p, hdr);
 	pkeyjump = JUMP(p, keyjump, LOCAL_JUMP, ALL_TRUE, SHRD | SELF);
 	/*