| Message ID | 20250210132740.20068-1-purvayeshi550@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | drivers: pci: Fix flexible array usage | expand |
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 >
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.
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
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; --
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.
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.
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?
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; > > -- >
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; }; /**
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(-)