powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()

Message ID 20190109102328.GB5476@kadam
State Accepted
Commit d7b6cc199b2dea602b4a2a681cf6d3223a61e2be
Headers show
Series
  • powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
snowpatch_ozlabs/build-pmac32 success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64e success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64be success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/build-ppc64le success build succeeded & removed 0 sparse warning(s)
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Dan Carpenter Jan. 9, 2019, 10:23 a.m.
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(-)

Comments

Michael Ellerman Jan. 9, 2019, 11:54 a.m. | #1
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
Alexey Kardashevskiy Jan. 11, 2019, 2:12 a.m. | #2
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
Balbir Singh Jan. 12, 2019, 12:30 a.m. | #3
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>
Dan Carpenter Jan. 12, 2019, 5:44 a.m. | #4
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
Segher Boessenkool Jan. 12, 2019, 7:34 a.m. | #5
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
Dan Carpenter Jan. 12, 2019, 10:51 a.m. | #6
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

Patch

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,