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

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

[Dan Carpenter]

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);
 		table_group = &pe->npucomp->table_group;
 		table_group->ops = &pnv_npu_peers_ops;
 		iommu_register_group(table_group, hose->global_number,
-- 
2.17.1

Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()

[Michael Ellerman]

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

Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()

[Alexey Kardashevskiy]

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

-- 
Alexey

Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()

[Balbir Singh]

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>

Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()

[Dan Carpenter]

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

Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()

[Segher Boessenkool]

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

Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()

[Dan Carpenter]

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

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

[Michael Ellerman]

On Wed, 2019-01-09 at 10:23:29 UTC, 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>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Acked-by: Balbir Singh <bsingharora@gmail.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/d7b6cc199b2dea602b4a2a681cf6d322

cheers