| Message ID | 20190109102328.GB5476@kadam (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d7b6cc199b2dea602b4a2a681cf6d3223a61e2be |
| Headers | show |
| Series | powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
Dan Carpenter <dan.carpenter@oracle.com> writes: > There is a typo so we accidentally allocate enough memory for a pointer > when we wanted to allocate enough for a struct. > > Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > arch/powerpc/platforms/powernv/npu-dma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, I've applied this to my fixes-test tree. Alexey can you send me an ack? cheers > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c > index d7f742ed48ba..3f58c7dbd581 100644 > --- a/arch/powerpc/platforms/powernv/npu-dma.c > +++ b/arch/powerpc/platforms/powernv/npu-dma.c > @@ -564,7 +564,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe) > } > } else { > /* Create a group for 1 GPU and attached NPUs for POWER8 */ > - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL); > + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL); > table_group = &pe->npucomp->table_group; > table_group->ops = &pnv_npu_peers_ops; > iommu_register_group(table_group, hose->global_number, > -- > 2.17.1
On 09/01/2019 22:54, Michael Ellerman wrote: > Dan Carpenter <dan.carpenter@oracle.com> writes: >> There is a typo so we accidentally allocate enough memory for a pointer >> when we wanted to allocate enough for a struct. >> >> Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> --- >> arch/powerpc/platforms/powernv/npu-dma.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks, I've applied this to my fixes-test tree. > > Alexey can you send me an ack? Ouch. Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > cheers > >> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c >> index d7f742ed48ba..3f58c7dbd581 100644 >> --- a/arch/powerpc/platforms/powernv/npu-dma.c >> +++ b/arch/powerpc/platforms/powernv/npu-dma.c >> @@ -564,7 +564,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe) >> } >> } else { >> /* Create a group for 1 GPU and attached NPUs for POWER8 */ >> - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL); >> + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL); >> table_group = &pe->npucomp->table_group; >> table_group->ops = &pnv_npu_peers_ops; >> iommu_register_group(table_group, hose->global_number, >> -- >> 2.17.1
On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote: > There is a typo so we accidentally allocate enough memory for a pointer > when we wanted to allocate enough for a struct. > > Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > arch/powerpc/platforms/powernv/npu-dma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c > index d7f742ed48ba..3f58c7dbd581 100644 > --- a/arch/powerpc/platforms/powernv/npu-dma.c > +++ b/arch/powerpc/platforms/powernv/npu-dma.c > @@ -564,7 +564,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe) > } > } else { > /* Create a group for 1 GPU and attached NPUs for POWER8 */ > - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL); > + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL); To avoid these in the future, I wonder if instead of sizeof(pe->npucomp), we insist on sizeof structure pe->npucomp = kzalloc(sizeof(struct npucomp), GFP_KERNEL); Acked-by: Balbir Singh <bsingharora@gmail.com>
On Sat, Jan 12, 2019 at 11:30:35AM +1100, Balbir Singh wrote: > On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote: > > There is a typo so we accidentally allocate enough memory for a pointer > > when we wanted to allocate enough for a struct. > > > > Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > arch/powerpc/platforms/powernv/npu-dma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c > > index d7f742ed48ba..3f58c7dbd581 100644 > > --- a/arch/powerpc/platforms/powernv/npu-dma.c > > +++ b/arch/powerpc/platforms/powernv/npu-dma.c > > @@ -564,7 +564,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe) > > } > > } else { > > /* Create a group for 1 GPU and attached NPUs for POWER8 */ > > - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL); > > + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL); > > To avoid these in the future, I wonder if instead of sizeof(pe->npucomp), we insist on > sizeof structure > > pe->npucomp = kzalloc(sizeof(struct npucomp), GFP_KERNEL); > The latest kernel fashion is sizeof(*ptr). It can go wrong either way. I don't have strong feelings about it. These sorts of bugs don't last long because they're caught in testing or with static analysis. regards, dan carpenter
On Sat, Jan 12, 2019 at 08:44:26AM +0300, Dan Carpenter wrote: > On Sat, Jan 12, 2019 at 11:30:35AM +1100, Balbir Singh wrote: > > On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote: > > > There is a typo so we accidentally allocate enough memory for a pointer > > > when we wanted to allocate enough for a struct. > > > > > > Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > arch/powerpc/platforms/powernv/npu-dma.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c > > > index d7f742ed48ba..3f58c7dbd581 100644 > > > --- a/arch/powerpc/platforms/powernv/npu-dma.c > > > +++ b/arch/powerpc/platforms/powernv/npu-dma.c > > > @@ -564,7 +564,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe) > > > } > > > } else { > > > /* Create a group for 1 GPU and attached NPUs for POWER8 */ > > > - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL); > > > + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL); > > > > To avoid these in the future, I wonder if instead of sizeof(pe->npucomp), we insist on > > sizeof structure > > > > pe->npucomp = kzalloc(sizeof(struct npucomp), GFP_KERNEL); > > > > The latest kernel fashion is sizeof(*ptr). It can go wrong either way. > I don't have strong feelings about it. These sorts of bugs don't last > long because they're caught in testing or with static analysis. And it is easy to see someone forgot the * in "sizeof *ptr", and with experience it will just automatically look wrong if it is forgotten; but it isn't obvious at all if the wrong struct is used, which cannot happen with the *ptr form, but happens frequently with the "sizeof(struct x)" form. Segher
On Sat, Jan 12, 2019 at 01:34:42AM -0600, Segher Boessenkool wrote: > On Sat, Jan 12, 2019 at 08:44:26AM +0300, Dan Carpenter wrote: > > On Sat, Jan 12, 2019 at 11:30:35AM +1100, Balbir Singh wrote: > > > On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote: > > > > There is a typo so we accidentally allocate enough memory for a pointer > > > > when we wanted to allocate enough for a struct. > > > > > > > > Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > --- > > > > arch/powerpc/platforms/powernv/npu-dma.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c > > > > index d7f742ed48ba..3f58c7dbd581 100644 > > > > --- a/arch/powerpc/platforms/powernv/npu-dma.c > > > > +++ b/arch/powerpc/platforms/powernv/npu-dma.c > > > > @@ -564,7 +564,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe) > > > > } > > > > } else { > > > > /* Create a group for 1 GPU and attached NPUs for POWER8 */ > > > > - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL); > > > > + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL); > > > > > > To avoid these in the future, I wonder if instead of sizeof(pe->npucomp), we insist on > > > sizeof structure > > > > > > pe->npucomp = kzalloc(sizeof(struct npucomp), GFP_KERNEL); > > > > > > > The latest kernel fashion is sizeof(*ptr). It can go wrong either way. > > I don't have strong feelings about it. These sorts of bugs don't last > > long because they're caught in testing or with static analysis. > > And it is easy to see someone forgot the * in "sizeof *ptr", and with > experience it will just automatically look wrong if it is forgotten; but > it isn't obvious at all if the wrong struct is used, which cannot happen > with the *ptr form, but happens frequently with the "sizeof(struct x)" > form. It doesn't happen very frequently. I look for this with Smatch. The results I see are when the first few members of a struct are a header and the actual size can vary. hdr = alloc(sizeof(struct larger_struct)); regards, dan carpenter
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c index d7f742ed48ba..3f58c7dbd581 100644 --- a/arch/powerpc/platforms/powernv/npu-dma.c +++ b/arch/powerpc/platforms/powernv/npu-dma.c @@ -564,7 +564,7 @@ struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe) } } else { /* Create a group for 1 GPU and attached NPUs for POWER8 */ - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL); + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL); table_group = &pe->npucomp->table_group; table_group->ops = &pnv_npu_peers_ops; iommu_register_group(table_group, hose->global_number,
There is a typo so we accidentally allocate enough memory for a pointer when we wanted to allocate enough for a struct. Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- arch/powerpc/platforms/powernv/npu-dma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)