| Message ID | 20191218170004.5297-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [-next] pci: fix a -Wtype-limits compilation warning | expand |
On 2019-12-18 10:00 a.m., Qian Cai wrote: > The commit a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") > introduced a compilation warning and a potential infinite loop because > it is no longer possible to be self-terminated as u8 is always less than > 256, > > In file included from ./include/linux/kernel.h:12, > from ./include/asm-generic/bug.h:19, > from ./arch/x86/include/asm/bug.h:83, > from ./include/linux/bug.h:5, > from ./include/linux/jump_label.h:250, > from ./arch/x86/include/asm/string_64.h:6, > from ./arch/x86/include/asm/string.h:5, > from ./include/linux/string.h:20, > from ./include/linux/uuid.h:12, > from ./include/linux/mod_devicetable.h:13, > from ./include/linux/pci.h:27, > from drivers/pci/search.c:11: > drivers/pci/search.c: In function 'pci_for_each_dma_alias': > ./include/linux/bitops.h:30:13: warning: comparison is always true due > to limited range of data type [-Wtype-limits] > (bit) < (size); \ > ^ > drivers/pci/search.c:46:3: note: in expansion of macro 'for_each_set_bit' > for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { > > Fixed it by using u16 for "devfn" in this occasion. > > Fixes: a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") > Signed-off-by: Qian Cai <cai@lca.pw> Makes sense to me, Reviewed-by: Logan Gunthorpe <logang@deltatee.com> > --- > drivers/pci/search.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c > index 9e4dfae47252..42bc44d0e681 100644 > --- a/drivers/pci/search.c > +++ b/drivers/pci/search.c > @@ -41,7 +41,7 @@ int pci_for_each_dma_alias(struct pci_dev *pdev, > * DMA, iterate over that too. > */ > if (unlikely(pdev->dma_alias_mask)) { > - u8 devfn; > + u16 devfn; > > for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { > ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn), >
On Wed, Dec 18, 2019 at 12:00:04PM -0500, Qian Cai wrote: > The commit a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") > introduced a compilation warning and a potential infinite loop because > it is no longer possible to be self-terminated as u8 is always less than > 256, > > In file included from ./include/linux/kernel.h:12, > from ./include/asm-generic/bug.h:19, > from ./arch/x86/include/asm/bug.h:83, > from ./include/linux/bug.h:5, > from ./include/linux/jump_label.h:250, > from ./arch/x86/include/asm/string_64.h:6, > from ./arch/x86/include/asm/string.h:5, > from ./include/linux/string.h:20, > from ./include/linux/uuid.h:12, > from ./include/linux/mod_devicetable.h:13, > from ./include/linux/pci.h:27, > from drivers/pci/search.c:11: > drivers/pci/search.c: In function 'pci_for_each_dma_alias': > ./include/linux/bitops.h:30:13: warning: comparison is always true due > to limited range of data type [-Wtype-limits] > (bit) < (size); \ > ^ > drivers/pci/search.c:46:3: note: in expansion of macro 'for_each_set_bit' > for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { > > Fixed it by using u16 for "devfn" in this occasion. Ugh, that is pretty subtle! Would you mind if we used "unsigned int" instead of "u16"? "u16" makes it look like just a mistake -- somebody is likely to come along and say devfn only needs "u8" and try to change it back. The same might happen with "unsigned int", but at least it doesn't look like it was chosen specifically to fit a devfn. Provisional patch below. > Fixes: a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") > Signed-off-by: Qian Cai <cai@lca.pw> > --- > drivers/pci/search.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/search.c b/drivers/pci/search.c > index 9e4dfae47252..42bc44d0e681 100644 > --- a/drivers/pci/search.c > +++ b/drivers/pci/search.c > @@ -41,7 +41,7 @@ int pci_for_each_dma_alias(struct pci_dev *pdev, > * DMA, iterate over that too. > */ > if (unlikely(pdev->dma_alias_mask)) { > - u8 devfn; > + u16 devfn; > > for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { > ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn), > -- > 2.21.0 (Apple Git-122.2) commit f8bf2aeb651b ("PCI: Fix pci_add_dma_alias() bitmask size") Author: James Sewart <jamessewart@arista.com> Date: Tue Dec 10 15:51:33 2019 -0600 PCI: Fix pci_add_dma_alias() bitmask size The number of possible devfns is 256, but pci_add_dma_alias() allocated a bitmap of size 255. Fix this off-by-one error. This fixes commits 338c3149a221 ("PCI: Add support for multiple DMA aliases") and c6635792737b ("PCI: Allocate dma_alias_mask with bitmap_zalloc()"), but I doubt it was possible to see a problem because it takes 4 64-bit longs (or 8 32-bit longs) to hold 255 bits, and bitmap_zalloc() doesn't save the 255-bit size anywhere. [bhelgaas: commit log, move #define to drivers/pci/pci.h, include loop limit fix from Qian Cai: https://lore.kernel.org/r/20191218170004.5297-1-cai@lca.pw] Signed-off-by: James Sewart <jamessewart@arista.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Logan Gunthorpe <logang@deltatee.com> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e87196cc1a7f..7b5fa2eabe09 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -6017,7 +6017,7 @@ EXPORT_SYMBOL_GPL(pci_pr3_present); void pci_add_dma_alias(struct pci_dev *dev, u8 devfn) { if (!dev->dma_alias_mask) - dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL); + dev->dma_alias_mask = bitmap_zalloc(MAX_NR_DEVFNS, GFP_KERNEL); if (!dev->dma_alias_mask) { pci_warn(dev, "Unable to allocate DMA alias mask\n"); return; diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index a0a53bd05a0b..6394e7746fb5 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -4,6 +4,9 @@ #include <linux/pci.h> +/* Number of possible devfns: 0.0 to 1f.7 inclusive */ +#define MAX_NR_DEVFNS 256 + #define PCI_FIND_CAP_TTL 48 #define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ diff --git a/drivers/pci/search.c b/drivers/pci/search.c index bade14002fd8..e4dbdef5aef0 100644 --- a/drivers/pci/search.c +++ b/drivers/pci/search.c @@ -41,9 +41,9 @@ int pci_for_each_dma_alias(struct pci_dev *pdev, * DMA, iterate over that too. */ if (unlikely(pdev->dma_alias_mask)) { - u8 devfn; + unsigned int devfn; - for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) { + for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn), data); if (ret)
On 2019-12-18 11:24 a.m., Bjorn Helgaas wrote: > On Wed, Dec 18, 2019 at 12:00:04PM -0500, Qian Cai wrote: >> The commit a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") >> introduced a compilation warning and a potential infinite loop because >> it is no longer possible to be self-terminated as u8 is always less than >> 256, >> >> In file included from ./include/linux/kernel.h:12, >> from ./include/asm-generic/bug.h:19, >> from ./arch/x86/include/asm/bug.h:83, >> from ./include/linux/bug.h:5, >> from ./include/linux/jump_label.h:250, >> from ./arch/x86/include/asm/string_64.h:6, >> from ./arch/x86/include/asm/string.h:5, >> from ./include/linux/string.h:20, >> from ./include/linux/uuid.h:12, >> from ./include/linux/mod_devicetable.h:13, >> from ./include/linux/pci.h:27, >> from drivers/pci/search.c:11: >> drivers/pci/search.c: In function 'pci_for_each_dma_alias': >> ./include/linux/bitops.h:30:13: warning: comparison is always true due >> to limited range of data type [-Wtype-limits] >> (bit) < (size); \ >> ^ >> drivers/pci/search.c:46:3: note: in expansion of macro 'for_each_set_bit' >> for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { >> >> Fixed it by using u16 for "devfn" in this occasion. > > Ugh, that is pretty subtle! Would you mind if we used "unsigned int" > instead of "u16"? "u16" makes it look like just a mistake -- somebody > is likely to come along and say devfn only needs "u8" and try to > change it back. The same might happen with "unsigned int", but at > least it doesn't look like it was chosen specifically to fit a devfn. I agree: you're right that we really don't need an int that's specifically sized here. There's really shouldn't be any gain in using a smaller integer, in theory "int" should be the most "efficient" anyway, just using a full register in hardware. Logan > Provisional patch below. > >> Fixes: a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") >> Signed-off-by: Qian Cai <cai@lca.pw> >> --- >> drivers/pci/search.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/pci/search.c b/drivers/pci/search.c >> index 9e4dfae47252..42bc44d0e681 100644 >> --- a/drivers/pci/search.c >> +++ b/drivers/pci/search.c >> @@ -41,7 +41,7 @@ int pci_for_each_dma_alias(struct pci_dev *pdev, >> * DMA, iterate over that too. >> */ >> if (unlikely(pdev->dma_alias_mask)) { >> - u8 devfn; >> + u16 devfn; >> >> for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { >> ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn), >> -- >> 2.21.0 (Apple Git-122.2) > > commit f8bf2aeb651b ("PCI: Fix pci_add_dma_alias() bitmask size") > Author: James Sewart <jamessewart@arista.com> > Date: Tue Dec 10 15:51:33 2019 -0600 > > PCI: Fix pci_add_dma_alias() bitmask size > > The number of possible devfns is 256, but pci_add_dma_alias() allocated a > bitmap of size 255. Fix this off-by-one error. > > This fixes commits 338c3149a221 ("PCI: Add support for multiple DMA > aliases") and c6635792737b ("PCI: Allocate dma_alias_mask with > bitmap_zalloc()"), but I doubt it was possible to see a problem because > it takes 4 64-bit longs (or 8 32-bit longs) to hold 255 bits, and > bitmap_zalloc() doesn't save the 255-bit size anywhere. > > [bhelgaas: commit log, move #define to drivers/pci/pci.h, include loop > limit fix from Qian Cai: > https://lore.kernel.org/r/20191218170004.5297-1-cai@lca.pw] > Signed-off-by: James Sewart <jamessewart@arista.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Reviewed-by: Logan Gunthorpe <logang@deltatee.com> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e87196cc1a7f..7b5fa2eabe09 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -6017,7 +6017,7 @@ EXPORT_SYMBOL_GPL(pci_pr3_present); > void pci_add_dma_alias(struct pci_dev *dev, u8 devfn) > { > if (!dev->dma_alias_mask) > - dev->dma_alias_mask = bitmap_zalloc(U8_MAX, GFP_KERNEL); > + dev->dma_alias_mask = bitmap_zalloc(MAX_NR_DEVFNS, GFP_KERNEL); > if (!dev->dma_alias_mask) { > pci_warn(dev, "Unable to allocate DMA alias mask\n"); > return; > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index a0a53bd05a0b..6394e7746fb5 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -4,6 +4,9 @@ > > #include <linux/pci.h> > > +/* Number of possible devfns: 0.0 to 1f.7 inclusive */ > +#define MAX_NR_DEVFNS 256 > + > #define PCI_FIND_CAP_TTL 48 > > #define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ > diff --git a/drivers/pci/search.c b/drivers/pci/search.c > index bade14002fd8..e4dbdef5aef0 100644 > --- a/drivers/pci/search.c > +++ b/drivers/pci/search.c > @@ -41,9 +41,9 @@ int pci_for_each_dma_alias(struct pci_dev *pdev, > * DMA, iterate over that too. > */ > if (unlikely(pdev->dma_alias_mask)) { > - u8 devfn; > + unsigned int devfn; > > - for_each_set_bit(devfn, pdev->dma_alias_mask, U8_MAX) { > + for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { > ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn), > data); > if (ret) >
> On Dec 18, 2019, at 1:24 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > Ugh, that is pretty subtle! Would you mind if we used "unsigned int" > instead of "u16"? "u16" makes it look like just a mistake -- somebody > is likely to come along and say devfn only needs "u8" and try to > change it back. The same might happen with "unsigned int", but at > least it doesn't look like it was chosen specifically to fit a devfn. > > Provisional patch below. Looks good to me.
diff --git a/drivers/pci/search.c b/drivers/pci/search.c index 9e4dfae47252..42bc44d0e681 100644 --- a/drivers/pci/search.c +++ b/drivers/pci/search.c @@ -41,7 +41,7 @@ int pci_for_each_dma_alias(struct pci_dev *pdev, * DMA, iterate over that too. */ if (unlikely(pdev->dma_alias_mask)) { - u8 devfn; + u16 devfn; for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { ret = fn(pdev, PCI_DEVID(pdev->bus->number, devfn),
The commit a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") introduced a compilation warning and a potential infinite loop because it is no longer possible to be self-terminated as u8 is always less than 256, In file included from ./include/linux/kernel.h:12, from ./include/asm-generic/bug.h:19, from ./arch/x86/include/asm/bug.h:83, from ./include/linux/bug.h:5, from ./include/linux/jump_label.h:250, from ./arch/x86/include/asm/string_64.h:6, from ./arch/x86/include/asm/string.h:5, from ./include/linux/string.h:20, from ./include/linux/uuid.h:12, from ./include/linux/mod_devicetable.h:13, from ./include/linux/pci.h:27, from drivers/pci/search.c:11: drivers/pci/search.c: In function 'pci_for_each_dma_alias': ./include/linux/bitops.h:30:13: warning: comparison is always true due to limited range of data type [-Wtype-limits] (bit) < (size); \ ^ drivers/pci/search.c:46:3: note: in expansion of macro 'for_each_set_bit' for_each_set_bit(devfn, pdev->dma_alias_mask, MAX_NR_DEVFNS) { Fixed it by using u16 for "devfn" in this occasion. Fixes: a7d06153eea2 ("PCI: Fix pci_add_dma_alias() bitmask size") Signed-off-by: Qian Cai <cai@lca.pw> --- drivers/pci/search.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)