[PATCH] drivers: pci: Fix flexible array usage

[PATCH] drivers: pci: Fix flexible array usage

[Purva Yeshi]

Fix warning detected by smatch tool:
Array of flexible structure occurs in 'pci_saved_state' struct

The warning occurs because struct pci_saved_state contains struct
pci_cap_saved_data cap[], where cap[] has a flexible array member (data[]).
Arrays of structures with flexible members are not allowed, leading to this
warning.

Replaced cap[] with a pointer (*cap), allowing dynamic memory allocation
instead of embedding an invalid array of flexible structures.

Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a7..648a080ef 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1929,7 +1929,7 @@ EXPORT_SYMBOL(pci_restore_state);
 
 struct pci_saved_state {
 	u32 config_space[16];
-	struct pci_cap_saved_data cap[];
+	struct pci_cap_saved_data *cap;
 };
 
 /**
-- 
2.34.1

Re: [PATCH] drivers: pci: Fix flexible array usage

[Bjorn Helgaas]

On Mon, Feb 10, 2025 at 06:57:40PM +0530, Purva Yeshi wrote:
> Fix warning detected by smatch tool:
> Array of flexible structure occurs in 'pci_saved_state' struct
> 
> The warning occurs because struct pci_saved_state contains struct
> pci_cap_saved_data cap[], where cap[] has a flexible array member (data[]).
> Arrays of structures with flexible members are not allowed, leading to this
> warning.
> 
> Replaced cap[] with a pointer (*cap), allowing dynamic memory allocation
> instead of embedding an invalid array of flexible structures.
> 
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>

Applied with subject:

  PCI: Fix array of flexible structure usage in struct pci_saved_state

to pci/enumeration for v6.15, thanks!

> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a7..648a080ef 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1929,7 +1929,7 @@ EXPORT_SYMBOL(pci_restore_state);
>  
>  struct pci_saved_state {
>  	u32 config_space[16];
> -	struct pci_cap_saved_data cap[];
> +	struct pci_cap_saved_data *cap;
>  };
>  
>  /**
> -- 
> 2.34.1
> 

Re: [PATCH] drivers: pci: Fix flexible array usage

[Keith Busch]

On Mon, Feb 10, 2025 at 06:57:40PM +0530, Purva Yeshi wrote:
> Fix warning detected by smatch tool:
> Array of flexible structure occurs in 'pci_saved_state' struct
> 
> The warning occurs because struct pci_saved_state contains struct
> pci_cap_saved_data cap[], where cap[] has a flexible array member (data[]).
> Arrays of structures with flexible members are not allowed, leading to this
> warning.
> 
> Replaced cap[] with a pointer (*cap), allowing dynamic memory allocation
> instead of embedding an invalid array of flexible structures.
> 
> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a7..648a080ef 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1929,7 +1929,7 @@ EXPORT_SYMBOL(pci_restore_state);
>  
>  struct pci_saved_state {
>  	u32 config_space[16];
> -	struct pci_cap_saved_data cap[];
> +	struct pci_cap_saved_data *cap;
>  };

I don't think this is right. Previously the space for "cap" was
allocated at the end of the pci_saved_state, but now it's just an
uninitialized pointer.

Re: [PATCH] drivers: pci: Fix flexible array usage

[Bjorn Helgaas]

On Mon, Feb 10, 2025 at 04:03:26PM -0700, Keith Busch wrote:
> On Mon, Feb 10, 2025 at 06:57:40PM +0530, Purva Yeshi wrote:
> > Fix warning detected by smatch tool:
> > Array of flexible structure occurs in 'pci_saved_state' struct
> > 
> > The warning occurs because struct pci_saved_state contains struct
> > pci_cap_saved_data cap[], where cap[] has a flexible array member (data[]).
> > Arrays of structures with flexible members are not allowed, leading to this
> > warning.
> > 
> > Replaced cap[] with a pointer (*cap), allowing dynamic memory allocation
> > instead of embedding an invalid array of flexible structures.
> > 
> > Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
> > ---
> >  drivers/pci/pci.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 869d204a7..648a080ef 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1929,7 +1929,7 @@ EXPORT_SYMBOL(pci_restore_state);
> >  
> >  struct pci_saved_state {
> >  	u32 config_space[16];
> > -	struct pci_cap_saved_data cap[];
> > +	struct pci_cap_saved_data *cap;
> >  };
> 
> I don't think this is right. Previously the space for "cap" was
> allocated at the end of the pci_saved_state, but now it's just an
> uninitialized pointer.

Thanks, I think you're right.  Dropped pending fix or better
explanation.

This is kind of a complicated data structure.  IIUC, a struct
pci_saved_state is allocated only in pci_store_saved_state(), where
the size is determined by the sum of the sizes of all the entries in
the dev->saved_cap_space list.

The pci_saved_state is filled by copying from entries in the
dev->saved_cap_space list.  The entries need not be all the same size
because we copy each entry manually based on its size.

So cap[] is really just the base of this buffer of variable-sized
entries.  Maybe "struct pci_cap_saved_data cap[]" is not the best
representation of this, but *cap (a pointer) doesn't seem better.

Bjorn

Re: [PATCH] drivers: pci: Fix flexible array usage

[Keith Busch]

On Tue, Feb 11, 2025 at 03:02:35PM -0600, Bjorn Helgaas wrote:
> This is kind of a complicated data structure.  IIUC, a struct
> pci_saved_state is allocated only in pci_store_saved_state(), where
> the size is determined by the sum of the sizes of all the entries in
> the dev->saved_cap_space list.
> 
> The pci_saved_state is filled by copying from entries in the
> dev->saved_cap_space list.  The entries need not be all the same size
> because we copy each entry manually based on its size.
> 
> So cap[] is really just the base of this buffer of variable-sized
> entries.  Maybe "struct pci_cap_saved_data cap[]" is not the best
> representation of this, but *cap (a pointer) doesn't seem better.

The original code is actually correct despite using a flexible array of
a struct that contains a flexible array. That arrangement just means you
can't index into it, but the code is only doing pointer arithmetic, so
should be fine.

With this struct:

struct pci_saved_state {
 	u32 config_space[16];
	struct pci_cap_saved_data cap[];
};

Accessing "cap" field returns the address right after the config_space[]
member. When it's changed to a pointer type, though, it needs to be
initialized to *something* but the code doesn't do that. The code just
expects the cap to follow right after the config.

Anyway, to silence the warning we can just make it an anonymous member
and add 1 to the state to get to the same place in memory as before.

---
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a37..e562037644fd0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1929,7 +1929,6 @@ EXPORT_SYMBOL(pci_restore_state);
 
 struct pci_saved_state {
 	u32 config_space[16];
-	struct pci_cap_saved_data cap[];
 };
 
 /**
@@ -1961,7 +1960,7 @@ struct pci_saved_state *pci_store_saved_state(struct pci_dev *dev)
 	memcpy(state->config_space, dev->saved_config_space,
 	       sizeof(state->config_space));
 
-	cap = state->cap;
+	cap = (void *)(state + 1);
 	hlist_for_each_entry(tmp, &dev->saved_cap_space, next) {
 		size_t len = sizeof(struct pci_cap_saved_data) + tmp->cap.size;
 		memcpy(cap, &tmp->cap, len);
@@ -1991,7 +1990,7 @@ int pci_load_saved_state(struct pci_dev *dev,
 	memcpy(dev->saved_config_space, state->config_space,
 	       sizeof(state->config_space));
 
-	cap = state->cap;
+	cap = (void *)(state + 1);
 	while (cap->size) {
 		struct pci_cap_saved_state *tmp;
 
--

Re: [PATCH] drivers: pci: Fix flexible array usage

[Purva Yeshi]

On 11/02/25 04:33, Keith Busch wrote:
> On Mon, Feb 10, 2025 at 06:57:40PM +0530, Purva Yeshi wrote:
>> Fix warning detected by smatch tool:
>> Array of flexible structure occurs in 'pci_saved_state' struct
>>
>> The warning occurs because struct pci_saved_state contains struct
>> pci_cap_saved_data cap[], where cap[] has a flexible array member (data[]).
>> Arrays of structures with flexible members are not allowed, leading to this
>> warning.
>>
>> Replaced cap[] with a pointer (*cap), allowing dynamic memory allocation
>> instead of embedding an invalid array of flexible structures.
>>
>> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
>> ---
>>   drivers/pci/pci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 869d204a7..648a080ef 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1929,7 +1929,7 @@ EXPORT_SYMBOL(pci_restore_state);
>>   
>>   struct pci_saved_state {
>>   	u32 config_space[16];
>> -	struct pci_cap_saved_data cap[];
>> +	struct pci_cap_saved_data *cap;
>>   };
> 
> I don't think this is right. Previously the space for "cap" was
> allocated at the end of the pci_saved_state, but now it's just an
> uninitialized pointer.

Thanks for your feedback. I understand your concern about the 
uninitialized pointer. To verify this, I tested the file using 
'~/smatch/smatch_scripts/kchecker drivers/pci/pci.c' smatch command, and 
it did not report any errors indicating that cap was uninitialized. 
Based on this, I initially believed the change to be safe.

Re: [PATCH] drivers: pci: Fix flexible array usage

[Purva Yeshi]

On 12/02/25 02:32, Bjorn Helgaas wrote:
> On Mon, Feb 10, 2025 at 04:03:26PM -0700, Keith Busch wrote:
>> On Mon, Feb 10, 2025 at 06:57:40PM +0530, Purva Yeshi wrote:
>>> Fix warning detected by smatch tool:
>>> Array of flexible structure occurs in 'pci_saved_state' struct
>>>
>>> The warning occurs because struct pci_saved_state contains struct
>>> pci_cap_saved_data cap[], where cap[] has a flexible array member (data[]).
>>> Arrays of structures with flexible members are not allowed, leading to this
>>> warning.
>>>
>>> Replaced cap[] with a pointer (*cap), allowing dynamic memory allocation
>>> instead of embedding an invalid array of flexible structures.
>>>
>>> Signed-off-by: Purva Yeshi <purvayeshi550@gmail.com>
>>> ---
>>>   drivers/pci/pci.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>> index 869d204a7..648a080ef 100644
>>> --- a/drivers/pci/pci.c
>>> +++ b/drivers/pci/pci.c
>>> @@ -1929,7 +1929,7 @@ EXPORT_SYMBOL(pci_restore_state);
>>>   
>>>   struct pci_saved_state {
>>>   	u32 config_space[16];
>>> -	struct pci_cap_saved_data cap[];
>>> +	struct pci_cap_saved_data *cap;
>>>   };
>>
>> I don't think this is right. Previously the space for "cap" was
>> allocated at the end of the pci_saved_state, but now it's just an
>> uninitialized pointer.
> 
> Thanks, I think you're right.  Dropped pending fix or better
> explanation.
> 
> This is kind of a complicated data structure.  IIUC, a struct
> pci_saved_state is allocated only in pci_store_saved_state(), where
> the size is determined by the sum of the sizes of all the entries in
> the dev->saved_cap_space list.
> 
> The pci_saved_state is filled by copying from entries in the
> dev->saved_cap_space list.  The entries need not be all the same size
> because we copy each entry manually based on its size.
> 
> So cap[] is really just the base of this buffer of variable-sized
> entries.  Maybe "struct pci_cap_saved_data cap[]" is not the best
> representation of this, but *cap (a pointer) doesn't seem better.
> 
> Bjorn

Thanks for the explanation. The primary goal of the patch was to address 
the Smatch warning regarding the flexible array member inside 
'pci_cap_saved_data'. However, from the explanation, I now got that the 
current approach may not be ideal.

Re: [PATCH] drivers: pci: Fix flexible array usage

[Purva Yeshi]

On 12/02/25 02:48, Keith Busch wrote:
> On Tue, Feb 11, 2025 at 03:02:35PM -0600, Bjorn Helgaas wrote:
>> This is kind of a complicated data structure.  IIUC, a struct
>> pci_saved_state is allocated only in pci_store_saved_state(), where
>> the size is determined by the sum of the sizes of all the entries in
>> the dev->saved_cap_space list.
>>
>> The pci_saved_state is filled by copying from entries in the
>> dev->saved_cap_space list.  The entries need not be all the same size
>> because we copy each entry manually based on its size.
>>
>> So cap[] is really just the base of this buffer of variable-sized
>> entries.  Maybe "struct pci_cap_saved_data cap[]" is not the best
>> representation of this, but *cap (a pointer) doesn't seem better.
> 
> The original code is actually correct despite using a flexible array of
> a struct that contains a flexible array. That arrangement just means you
> can't index into it, but the code is only doing pointer arithmetic, so
> should be fine.
> 
> With this struct:
> 
> struct pci_saved_state {
>   	u32 config_space[16];
> 	struct pci_cap_saved_data cap[];
> };
> 
> Accessing "cap" field returns the address right after the config_space[]
> member. When it's changed to a pointer type, though, it needs to be
> initialized to *something* but the code doesn't do that. The code just
> expects the cap to follow right after the config.
> 
> Anyway, to silence the warning we can just make it an anonymous member
> and add 1 to the state to get to the same place in memory as before.
> 
> ---
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a37..e562037644fd0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1929,7 +1929,6 @@ EXPORT_SYMBOL(pci_restore_state);
>   
>   struct pci_saved_state {
>   	u32 config_space[16];
> -	struct pci_cap_saved_data cap[];
>   };
>   
>   /**
> @@ -1961,7 +1960,7 @@ struct pci_saved_state *pci_store_saved_state(struct pci_dev *dev)
>   	memcpy(state->config_space, dev->saved_config_space,
>   	       sizeof(state->config_space));
>   
> -	cap = state->cap;
> +	cap = (void *)(state + 1);
>   	hlist_for_each_entry(tmp, &dev->saved_cap_space, next) {
>   		size_t len = sizeof(struct pci_cap_saved_data) + tmp->cap.size;
>   		memcpy(cap, &tmp->cap, len);
> @@ -1991,7 +1990,7 @@ int pci_load_saved_state(struct pci_dev *dev,
>   	memcpy(dev->saved_config_space, state->config_space,
>   	       sizeof(state->config_space));
>   
> -	cap = state->cap;
> +	cap = (void *)(state + 1);
>   	while (cap->size) {
>   		struct pci_cap_saved_state *tmp;
>   
> --

Thanks for the clarification. I now see that the original use of a 
flexible array inside 'pci_saved_state' is correct. Would you like me to 
resend the patch with the modifications you suggested?

Re: [PATCH] drivers: pci: Fix flexible array usage

[Ilpo Järvinen]

On Tue, 11 Feb 2025, Keith Busch wrote:

> On Tue, Feb 11, 2025 at 03:02:35PM -0600, Bjorn Helgaas wrote:
> > This is kind of a complicated data structure.  IIUC, a struct
> > pci_saved_state is allocated only in pci_store_saved_state(), where
> > the size is determined by the sum of the sizes of all the entries in
> > the dev->saved_cap_space list.
> > 
> > The pci_saved_state is filled by copying from entries in the
> > dev->saved_cap_space list.  The entries need not be all the same size
> > because we copy each entry manually based on its size.
> > 
> > So cap[] is really just the base of this buffer of variable-sized
> > entries.  Maybe "struct pci_cap_saved_data cap[]" is not the best
> > representation of this, but *cap (a pointer) doesn't seem better.
> 
> The original code is actually correct despite using a flexible array of
> a struct that contains a flexible array. That arrangement just means you
> can't index into it, but the code is only doing pointer arithmetic, so
> should be fine.
> 
> With this struct:
> 
> struct pci_saved_state {
>  	u32 config_space[16];
> 	struct pci_cap_saved_data cap[];
> };
> 
> Accessing "cap" field returns the address right after the config_space[]
> member. When it's changed to a pointer type, though, it needs to be
> initialized to *something* but the code doesn't do that. The code just
> expects the cap to follow right after the config.
> 
> Anyway, to silence the warning we can just make it an anonymous member
> and add 1 to the state to get to the same place in memory as before.
> 
> ---
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a37..e562037644fd0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1929,7 +1929,6 @@ EXPORT_SYMBOL(pci_restore_state);
>  
>  struct pci_saved_state {
>  	u32 config_space[16];
> -	struct pci_cap_saved_data cap[];

Can't just [] be dropped from it (and removed from the size calculation)?

It's not a real flex array because the second pci_cap_saved_data is not at 
->cap[1] but calculated by the loop by adding in the data in between. But 
there's one entry at ->cap[0] which is same as ->cap, no need to make it 
a flex array at all, IMO.

>  };
>  
>  /**
> @@ -1961,7 +1960,7 @@ struct pci_saved_state *pci_store_saved_state(struct pci_dev *dev)
>  	memcpy(state->config_space, dev->saved_config_space,
>  	       sizeof(state->config_space));
>  
> -	cap = state->cap;
> +	cap = (void *)(state + 1);
>  	hlist_for_each_entry(tmp, &dev->saved_cap_space, next) {
>  		size_t len = sizeof(struct pci_cap_saved_data) + tmp->cap.size;
>  		memcpy(cap, &tmp->cap, len);
> @@ -1991,7 +1990,7 @@ int pci_load_saved_state(struct pci_dev *dev,
>  	memcpy(dev->saved_config_space, state->config_space,
>  	       sizeof(state->config_space));
>  
> -	cap = state->cap;
> +	cap = (void *)(state + 1);
>  	while (cap->size) {
>  		struct pci_cap_saved_state *tmp;
>  
> --
> 

-- 
 i.