diff mbox series

[v2] dmaengine: pl08x: Use kcalloc() instead of kzalloc()

Message ID AS8PR02MB72373D9261B3B166048A8E218B392@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive)
State Accepted
Commit 98f2233a5c20ca567b2db1147278fd110681b9ed
Headers show
Series [v2] dmaengine: pl08x: Use kcalloc() instead of kzalloc() | expand

Commit Message

Erick Archer March 30, 2024, 3:23 p.m. UTC
This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1].

Here the multiplication is obviously safe because the "channels"
member can only be 8 or 2. This value is set when the "vendor_data"
structs are initialized.

static struct vendor_data vendor_pl080 = {
	[...]
	.channels = 8,
	[...]
};

static struct vendor_data vendor_nomadik = {
	[...]
	.channels = 8,
	[...]
};

static struct vendor_data vendor_pl080s = {
	[...]
	.channels = 8,
	[...]
};

static struct vendor_data vendor_pl081 = {
	[...]
	.channels = 2,
	[...]
};

However, using kcalloc() is more appropriate [1] and improves
readability. This patch has no effect on runtime behavior.

Link: https://github.com/KSPP/linux/issues/162 [1]
Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Erick Archer <erick.archer@outlook.com>
---
Changes in v2:
- Add the "Reviewed-by:" tag.
- Rebase against linux-next.

Previous versions:
v1 -> https://lore.kernel.org/linux-hardening/20240128115236.4791-1-erick.archer@gmx.com/

Hi everyone,

This patch seems to be lost. Gustavo reviewed it on January 30, 2024
but the patch has not been applied since.

Thanks,
Erick
---
 drivers/dma/amba-pl08x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Vinod Koul April 7, 2024, 11:39 a.m. UTC | #1
On 30-03-24, 16:23, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1].
> 
> Here the multiplication is obviously safe because the "channels"
> member can only be 8 or 2. This value is set when the "vendor_data"
> structs are initialized.
> 
> static struct vendor_data vendor_pl080 = {
> 	[...]
> 	.channels = 8,
> 	[...]
> };
> 
> static struct vendor_data vendor_nomadik = {
> 	[...]
> 	.channels = 8,
> 	[...]
> };
> 
> static struct vendor_data vendor_pl080s = {
> 	[...]
> 	.channels = 8,
> 	[...]
> };
> 
> static struct vendor_data vendor_pl081 = {
> 	[...]
> 	.channels = 2,
> 	[...]
> };
> 
> However, using kcalloc() is more appropriate [1] and improves
> readability. This patch has no effect on runtime behavior.
> 
> Link: https://github.com/KSPP/linux/issues/162 [1]
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Erick Archer <erick.archer@outlook.com>
> ---
> Changes in v2:
> - Add the "Reviewed-by:" tag.
> - Rebase against linux-next.
> 
> Previous versions:
> v1 -> https://lore.kernel.org/linux-hardening/20240128115236.4791-1-erick.archer@gmx.com/
> 
> Hi everyone,
> 
> This patch seems to be lost. Gustavo reviewed it on January 30, 2024
> but the patch has not been applied since.
> 
> Thanks,
> Erick
> ---
>  drivers/dma/amba-pl08x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index fbf048f432bf..73a5cfb4da8a 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -2855,8 +2855,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
>  	}
>  
>  	/* Initialize physical channels */
> -	pl08x->phy_chans = kzalloc((vd->channels * sizeof(*pl08x->phy_chans)),
> -			GFP_KERNEL);
> +	pl08x->phy_chans = kcalloc(vd->channels, sizeof(*pl08x->phy_chans),
> +				   GFP_KERNEL);

How is one better than the other?


>  	if (!pl08x->phy_chans) {
>  		ret = -ENOMEM;
>  		goto out_no_phychans;
> -- 
> 2.25.1
Erick Archer April 7, 2024, 5:22 p.m. UTC | #2
Hi Vinod,

On Sun, Apr 07, 2024 at 05:09:39PM +0530, Vinod Koul wrote:
> On 30-03-24, 16:23, Erick Archer wrote:
> > This is an effort to get rid of all multiplications from allocation
> > functions in order to prevent integer overflows [1].
> > 
> > Here the multiplication is obviously safe because the "channels"
> > member can only be 8 or 2. This value is set when the "vendor_data"
> > structs are initialized.
> > 
> > static struct vendor_data vendor_pl080 = {
> > 	[...]
> > 	.channels = 8,
> > 	[...]
> > };
> > 
> > static struct vendor_data vendor_nomadik = {
> > 	[...]
> > 	.channels = 8,
> > 	[...]
> > };
> > 
> > static struct vendor_data vendor_pl080s = {
> > 	[...]
> > 	.channels = 8,
> > 	[...]
> > };
> > 
> > static struct vendor_data vendor_pl081 = {
> > 	[...]
> > 	.channels = 2,
> > 	[...]
> > };
> > 
> > However, using kcalloc() is more appropriate [1] and improves
> > readability. This patch has no effect on runtime behavior.
> > 
> > Link: https://github.com/KSPP/linux/issues/162 [1]
> > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
> > Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > Signed-off-by: Erick Archer <erick.archer@outlook.com>
> > ---
> > Changes in v2:
> > - Add the "Reviewed-by:" tag.
> > - Rebase against linux-next.
> > 
> > Previous versions:
> > v1 -> https://lore.kernel.org/linux-hardening/20240128115236.4791-1-erick.archer@gmx.com/
> > 
> > Hi everyone,
> > 
> > This patch seems to be lost. Gustavo reviewed it on January 30, 2024
> > but the patch has not been applied since.
> > 
> > Thanks,
> > Erick
> > ---
> >  drivers/dma/amba-pl08x.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> > index fbf048f432bf..73a5cfb4da8a 100644
> > --- a/drivers/dma/amba-pl08x.c
> > +++ b/drivers/dma/amba-pl08x.c
> > @@ -2855,8 +2855,8 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
> >  	}
> >  
> >  	/* Initialize physical channels */
> > -	pl08x->phy_chans = kzalloc((vd->channels * sizeof(*pl08x->phy_chans)),
> > -			GFP_KERNEL);
> > +	pl08x->phy_chans = kcalloc(vd->channels, sizeof(*pl08x->phy_chans),
> > +				   GFP_KERNEL);
> 
> How is one better than the other?
> 
The advantage of kcalloc is that it will prevent integer overflows that
could result from the multiplication of number of elements and size, and
it is also a bit nicer to read.

However, in this case the multiplication is obviously safe but the best
practice is to use the two factor form if available (as explained in the
section "open-coded arithmetic in allocator arguments" of the "Deprecated
Interfaces, Language Features, Attributes, and Conventions [1]"

[1] https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments

Regards,
Erick
> 
> >  	if (!pl08x->phy_chans) {
> >  		ret = -ENOMEM;
> >  		goto out_no_phychans;
> > -- 
> > 2.25.1
> 
> -- 
> ~Vinod
Vinod Koul April 18, 2024, 5:13 a.m. UTC | #3
On Sat, 30 Mar 2024 16:23:23 +0100, Erick Archer wrote:
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1].
> 
> Here the multiplication is obviously safe because the "channels"
> member can only be 8 or 2. This value is set when the "vendor_data"
> structs are initialized.
> 
> [...]

Applied, thanks!

[1/1] dmaengine: pl08x: Use kcalloc() instead of kzalloc()
      commit: 98f2233a5c20ca567b2db1147278fd110681b9ed

Best regards,
diff mbox series

Patch

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index fbf048f432bf..73a5cfb4da8a 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -2855,8 +2855,8 @@  static int pl08x_probe(struct amba_device *adev, const struct amba_id *id)
 	}
 
 	/* Initialize physical channels */
-	pl08x->phy_chans = kzalloc((vd->channels * sizeof(*pl08x->phy_chans)),
-			GFP_KERNEL);
+	pl08x->phy_chans = kcalloc(vd->channels, sizeof(*pl08x->phy_chans),
+				   GFP_KERNEL);
 	if (!pl08x->phy_chans) {
 		ret = -ENOMEM;
 		goto out_no_phychans;