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

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 <redacted>
---
 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

Dan Carpenter [off-list ref] 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 <redacted>
---
 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

On 09/01/2019 22:54, Michael Ellerman wrote:

Dan Carpenter [off-list ref] writes:

quoted

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 <redacted>
---
 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 <redacted>

cheers

quoted

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

On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote:

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:

quoted

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 <redacted>
---
 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:

quoted

On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote:

quoted

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 <redacted>
---
 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:

quoted

On Sat, Jan 12, 2019 at 11:30:35AM +1100, Balbir Singh wrote:

quoted

On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote:

quoted

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 <redacted>
---
 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

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 <redacted>
Reviewed-by: Alexey Kardashevskiy <redacted>
Acked-by: Balbir Singh <bsingharora@gmail.com>

Applied to powerpc fixes, thanks.

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

cheers