LinuxPPC-Dev Archive on lore.kernel.org
 help / Atom feed
* [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
@ 2019-01-09 10:23 Dan Carpenter
  2019-01-09 11:54 ` Michael Ellerman
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Dan Carpenter @ 2019-01-09 10:23 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Alexey Kardashevskiy
  Cc: Alexey Kardashevskiy, Mark Hairgrove, kernel-janitors,
	Paul Mackerras, Alistair Popple, linuxppc-dev

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
  2019-01-09 10:23 [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group() Dan Carpenter
@ 2019-01-09 11:54 ` Michael Ellerman
  2019-01-11  2:12   ` Alexey Kardashevskiy
  2019-01-12  0:30 ` Balbir Singh
  2019-01-14 10:12 ` Michael Ellerman
  2 siblings, 1 reply; 8+ messages in thread
From: Michael Ellerman @ 2019-01-09 11:54 UTC (permalink / raw)
  To: Dan Carpenter, Benjamin Herrenschmidt, Alexey Kardashevskiy
  Cc: Alexey Kardashevskiy, Alistair Popple, Mark Hairgrove,
	kernel-janitors, Paul Mackerras, linuxppc-dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
  2019-01-09 11:54 ` Michael Ellerman
@ 2019-01-11  2:12   ` Alexey Kardashevskiy
  0 siblings, 0 replies; 8+ messages in thread
From: Alexey Kardashevskiy @ 2019-01-11  2:12 UTC (permalink / raw)
  To: Michael Ellerman, Dan Carpenter, Benjamin Herrenschmidt
  Cc: Alistair Popple, Mark Hairgrove, kernel-janitors, Paul Mackerras,
	linuxppc-dev



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
  2019-01-09 10:23 [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group() Dan Carpenter
  2019-01-09 11:54 ` Michael Ellerman
@ 2019-01-12  0:30 ` Balbir Singh
  2019-01-12  5:44   ` Dan Carpenter
  2019-01-14 10:12 ` Michael Ellerman
  2 siblings, 1 reply; 8+ messages in thread
From: Balbir Singh @ 2019-01-12  0:30 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Alexey Kardashevskiy, Mark Hairgrove, kernel-janitors,
	Paul Mackerras, Alistair Popple, linuxppc-dev

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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
  2019-01-12  0:30 ` Balbir Singh
@ 2019-01-12  5:44   ` Dan Carpenter
  2019-01-12  7:34     ` Segher Boessenkool
  0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2019-01-12  5:44 UTC (permalink / raw)
  To: Balbir Singh
  Cc: Alexey Kardashevskiy, Mark Hairgrove, kernel-janitors,
	Paul Mackerras, Alistair Popple, linuxppc-dev

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
  2019-01-12  5:44   ` Dan Carpenter
@ 2019-01-12  7:34     ` Segher Boessenkool
  2019-01-12 10:51       ` Dan Carpenter
  0 siblings, 1 reply; 8+ messages in thread
From: Segher Boessenkool @ 2019-01-12  7:34 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Alexey Kardashevskiy, Alistair Popple, Mark Hairgrove,
	kernel-janitors, Paul Mackerras, linuxppc-dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
  2019-01-12  7:34     ` Segher Boessenkool
@ 2019-01-12 10:51       ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2019-01-12 10:51 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: Alexey Kardashevskiy, Alistair Popple, Mark Hairgrove,
	kernel-janitors, Paul Mackerras, linuxppc-dev

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
  2019-01-09 10:23 [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group() Dan Carpenter
  2019-01-09 11:54 ` Michael Ellerman
  2019-01-12  0:30 ` Balbir Singh
@ 2019-01-14 10:12 ` Michael Ellerman
  2 siblings, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2019-01-14 10:12 UTC (permalink / raw)
  To: Dan Carpenter, Benjamin Herrenschmidt, Alexey Kardashevskiy
  Cc: Alexey Kardashevskiy, Alistair Popple, Mark Hairgrove,
	kernel-janitors, Paul Mackerras, linuxppc-dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-09 10:23 [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group() Dan Carpenter
2019-01-09 11:54 ` Michael Ellerman
2019-01-11  2:12   ` Alexey Kardashevskiy
2019-01-12  0:30 ` Balbir Singh
2019-01-12  5:44   ` Dan Carpenter
2019-01-12  7:34     ` Segher Boessenkool
2019-01-12 10:51       ` Dan Carpenter
2019-01-14 10:12 ` Michael Ellerman

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org linuxppc-dev@archiver.kernel.org
	public-inbox-index linuxppc-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox