[v4,10/10] common/dpaxx: remove zero length array
Checks
Commit Message
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
> 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
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.
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.
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?
> -----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.
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.
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);
/*
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
@@ -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);
/*