[PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[Fabian Frederick]

kmalloc_array manages count*sizeof overflow.

Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
 net/dsa/dsa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 4648f12..c00cca3 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -526,7 +526,8 @@ static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
 
 	/* First time routing table allocation */
 	if (!cd->rtable) {
-		cd->rtable = kmalloc(pd->nr_chips * sizeof(s8), GFP_KERNEL);
+		cd->rtable = kmalloc_array(pd->nr_chips, sizeof(s8),
+					   GFP_KERNEL);
 		if (!cd->rtable)
 			return -ENOMEM;
 
-- 
1.9.3

Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[Joe Perches]

On Fri, 2014-11-14 at 19:36 +0100, Fabian Frederick wrote:
> kmalloc_array manages count*sizeof overflow.

Fundamentally correct, but is this necessary or useful?
sizeof(s8) isn't often going to be anything other than 1.

Would the kernel even work without that assumption?


> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
[]
> @@ -526,7 +526,8 @@ static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
>  
>  	/* First time routing table allocation */
>  	if (!cd->rtable) {
> -		cd->rtable = kmalloc(pd->nr_chips * sizeof(s8), GFP_KERNEL);
> +		cd->rtable = kmalloc_array(pd->nr_chips, sizeof(s8),
> +					   GFP_KERNEL);
>  		if (!cd->rtable)
>  			return -ENOMEM;
>  

Maybe all of these could be simplified

$ git grep -E "\*\s*sizeof\s*\(\s*[us]8\s*\)"
arch/arm/common/edma.c:                                   (edma_cc->num_tc + 1) * sizeof(s8),
drivers/acpi/utils.c:                               (element->buffer.length * sizeof(u8));
drivers/acpi/utils.c:                           tail += element->buffer.length * sizeof(u8);
drivers/char/tpm/tpm_i2c_stm_st33.c:        kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
drivers/char/tpm/tpm_i2c_stm_st33.c:        kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
drivers/gpu/drm/r128/r128_state.c:      mask_size = depth->n * sizeof(u8);
drivers/gpu/drm/r128/r128_state.c:              mask_size = depth->n * sizeof(u8);
drivers/iio/common/st_sensors/st_sensors_spi.c: memcpy(data, tb->rx_buf, len*sizeof(u8));
drivers/infiniband/hw/amso1100/c2_mq.h: u8 pad[64 - sizeof(u16) - 2 * sizeof(u8) - sizeof(u32) - sizeof(u16)];
drivers/input/tablet/aiptek.c:  const int sizeof_buf = 3 * sizeof(u8);
drivers/input/tablet/aiptek.c:  const int sizeof_buf = 3 * sizeof(u8);
drivers/md/dm-crypt.c:  memset(&cc->key, 0, cc->key_size * sizeof(u8));
drivers/md/dm-crypt.c:  cc = kzalloc(sizeof(*cc) + key_size * sizeof(u8), GFP_KERNEL);
drivers/media/dvb-frontends/dib7000p.c: tx = kzalloc(2*sizeof(u8), GFP_KERNEL);
drivers/media/dvb-frontends/dib7000p.c: rx = kzalloc(2*sizeof(u8), GFP_KERNEL);
drivers/media/dvb-frontends/dib8000.c:  client.i2c_write_buffer = kzalloc(4 * sizeof(u8), GFP_KERNEL);
drivers/media/dvb-frontends/dib8000.c:  client.i2c_read_buffer = kzalloc(4 * sizeof(u8), GFP_KERNEL);
drivers/media/dvb-frontends/dib9000.c:  client.i2c_write_buffer = kzalloc(4 * sizeof(u8), GFP_KERNEL);
drivers/media/dvb-frontends/dib9000.c:  client.i2c_read_buffer = kzalloc(4 * sizeof(u8), GFP_KERNEL);
drivers/media/pci/ttpci/av7110_ipack.c: if (!(p->buf = vmalloc(size*sizeof(u8)))) {
drivers/mtd/inftlmount.c:                               s->nb_blocks * sizeof(u8));
drivers/net/wireless/ath/ath10k/htt.h:   *  b) num_chars * sizeof(u8) aligned to 4bytes */
drivers/net/wireless/b43/ppr.c: BUILD_BUG_ON(sizeof(struct b43_ppr) != B43_PPR_RATES_NUM * sizeof(u8));
drivers/net/wireless/iwlwifi/pcie/trans.c:                     trans_pcie->n_no_reclaim_cmds * sizeof(u8));
drivers/net/wireless/rtlwifi/efuse.c:   memset(data, 0xff, PGPKT_DATA_SIZE * sizeof(u8));
drivers/net/wireless/rtlwifi/efuse.c:   memset(tmpdata, 0xff, PGPKT_DATA_SIZE * sizeof(u8));
drivers/net/wireless/rtlwifi/efuse.c:   u8 originaldata[8 * sizeof(u8)];
drivers/net/wireless/rtlwifi/efuse.c:   u8 originaldata[8 * sizeof(u8)];
drivers/net/wireless/rtlwifi/efuse.c:           memset(originaldata, 0xff,  8 * sizeof(u8));
drivers/net/wireless/rtlwifi/efuse.c:   memset(target_pkt.data, 0xFF,  8 * sizeof(u8));
drivers/power/ds2781_battery.c: ret = w1_ds2781_read(dev_info, val, DS2781_VOLT_MSB, 2 * sizeof(u8));
drivers/power/ds2781_battery.c: ret = w1_ds2781_read(dev_info, val, DS2781_TEMP_MSB, 2 * sizeof(u8));
drivers/rtc/rtc-pcf2123.c:      ret = spi_write(spi, txbuf, 2 * sizeof(u8));
drivers/rtc/rtc-pcf2123.c:      ret = spi_write(spi, txbuf, 2 * sizeof(u8));
drivers/rtc/rtc-pcf2123.c:      ret = spi_write_then_read(spi, txbuf, 1 * sizeof(u8),
drivers/rtc/rtc-pcf2123.c:                                      rxbuf, 2 * sizeof(u8));
drivers/thermal/x86_pkg_temp_thermal.c:                 (max_phy_id+1) * sizeof(u8), GFP_ATOMIC);
fs/compat_ioctl.c:      if (__copy_in_user(&tdata->read_write, &udata->read_write, 2 * sizeof(u8)))
net/dsa/dsa.c:          cd->rtable = kmalloc(pd->nr_chips * sizeof(s8), GFP_KERNEL);
net/dsa/dsa.c:          memset(cd->rtable, -1, pd->nr_chips * sizeof(s8));

Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[Fabian Frederick]

> On 14 November 2014 at 19:47 Joe Perches <joe@perches.com> wrote:
>
>
> On Fri, 2014-11-14 at 19:36 +0100, Fabian Frederick wrote:
> > kmalloc_array manages count*sizeof overflow.
>
> Fundamentally correct, but is this necessary or useful?
> sizeof(s8) isn't often going to be anything other than 1.
Absolutely, I thought it was a struct :)

There must be a reason for so many cases though ...

Regards,
Fabian

>
> Would the kernel even work without that assumption?
>
>
> > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> []
> > @@ -526,7 +526,8 @@ static int dsa_of_setup_routing_table(struct
> > dsa_platform_data *pd,
> > 
> >     /* First time routing table allocation */
> >     if (!cd->rtable) {
> > -           cd->rtable = kmalloc(pd->nr_chips * sizeof(s8), GFP_KERNEL);
> > +           cd->rtable = kmalloc_array(pd->nr_chips, sizeof(s8),
> > +                                      GFP_KERNEL);
> >             if (!cd->rtable)
> >                     return -ENOMEM;
> > 
>
> Maybe all of these could be simplified
>
> $ git grep -E "\*\s*sizeof\s*\(\s*[us]8\s*\)"
> arch/arm/common/edma.c:                                   (edma_cc->num_tc +
> 1) * sizeof(s8),
> drivers/acpi/utils.c:                               (element->buffer.length *
> sizeof(u8));
> drivers/acpi/utils.c:                           tail += element->buffer.length
> * sizeof(u8);
> drivers/char/tpm/tpm_i2c_stm_st33.c:        kmalloc(TPM_BUFSIZE * sizeof(u8),
> GFP_KERNEL);
> drivers/char/tpm/tpm_i2c_stm_st33.c:        kmalloc(TPM_BUFSIZE * sizeof(u8),
> GFP_KERNEL);
> drivers/gpu/drm/r128/r128_state.c:      mask_size = depth->n * sizeof(u8);
> drivers/gpu/drm/r128/r128_state.c:              mask_size = depth->n *
> sizeof(u8);
> drivers/iio/common/st_sensors/st_sensors_spi.c: memcpy(data, tb->rx_buf,
> len*sizeof(u8));
> drivers/infiniband/hw/amso1100/c2_mq.h: u8 pad[64 - sizeof(u16) - 2 *
> sizeof(u8) - sizeof(u32) - sizeof(u16)];
> drivers/input/tablet/aiptek.c:  const int sizeof_buf = 3 * sizeof(u8);
> drivers/input/tablet/aiptek.c:  const int sizeof_buf = 3 * sizeof(u8);
> drivers/md/dm-crypt.c:  memset(&cc->key, 0, cc->key_size * sizeof(u8));
> drivers/md/dm-crypt.c:  cc = kzalloc(sizeof(*cc) + key_size * sizeof(u8),
> GFP_KERNEL);
> drivers/media/dvb-frontends/dib7000p.c: tx = kzalloc(2*sizeof(u8),
> GFP_KERNEL);
> drivers/media/dvb-frontends/dib7000p.c: rx = kzalloc(2*sizeof(u8),
> GFP_KERNEL);
> drivers/media/dvb-frontends/dib8000.c:  client.i2c_write_buffer = kzalloc(4 *
> sizeof(u8), GFP_KERNEL);
> drivers/media/dvb-frontends/dib8000.c:  client.i2c_read_buffer = kzalloc(4 *
> sizeof(u8), GFP_KERNEL);
> drivers/media/dvb-frontends/dib9000.c:  client.i2c_write_buffer = kzalloc(4 *
> sizeof(u8), GFP_KERNEL);
> drivers/media/dvb-frontends/dib9000.c:  client.i2c_read_buffer = kzalloc(4 *
> sizeof(u8), GFP_KERNEL);
> drivers/media/pci/ttpci/av7110_ipack.c: if (!(p->buf =
> vmalloc(size*sizeof(u8)))) {
> drivers/mtd/inftlmount.c:                               s->nb_blocks *
> sizeof(u8));
> drivers/net/wireless/ath/ath10k/htt.h:   *  b) num_chars * sizeof(u8) aligned
> to 4bytes */
> drivers/net/wireless/b43/ppr.c: BUILD_BUG_ON(sizeof(struct b43_ppr) !=
> B43_PPR_RATES_NUM * sizeof(u8));
> drivers/net/wireless/iwlwifi/pcie/trans.c:                   
>  trans_pcie->n_no_reclaim_cmds * sizeof(u8));
> drivers/net/wireless/rtlwifi/efuse.c:   memset(data, 0xff, PGPKT_DATA_SIZE *
> sizeof(u8));
> drivers/net/wireless/rtlwifi/efuse.c:   memset(tmpdata, 0xff, PGPKT_DATA_SIZE
> * sizeof(u8));
> drivers/net/wireless/rtlwifi/efuse.c:   u8 originaldata[8 * sizeof(u8)];
> drivers/net/wireless/rtlwifi/efuse.c:   u8 originaldata[8 * sizeof(u8)];
> drivers/net/wireless/rtlwifi/efuse.c:           memset(originaldata, 0xff,  8
> * sizeof(u8));
> drivers/net/wireless/rtlwifi/efuse.c:   memset(target_pkt.data, 0xFF,  8 *
> sizeof(u8));
> drivers/power/ds2781_battery.c: ret = w1_ds2781_read(dev_info, val,
> DS2781_VOLT_MSB, 2 * sizeof(u8));
> drivers/power/ds2781_battery.c: ret = w1_ds2781_read(dev_info, val,
> DS2781_TEMP_MSB, 2 * sizeof(u8));
> drivers/rtc/rtc-pcf2123.c:      ret = spi_write(spi, txbuf, 2 * sizeof(u8));
> drivers/rtc/rtc-pcf2123.c:      ret = spi_write(spi, txbuf, 2 * sizeof(u8));
> drivers/rtc/rtc-pcf2123.c:      ret = spi_write_then_read(spi, txbuf, 1 *
> sizeof(u8),
> drivers/rtc/rtc-pcf2123.c:                                      rxbuf, 2 *
> sizeof(u8));
> drivers/thermal/x86_pkg_temp_thermal.c:                 (max_phy_id+1) *
> sizeof(u8), GFP_ATOMIC);
> fs/compat_ioctl.c:      if (__copy_in_user(&tdata->read_write,
> &udata->read_write, 2 * sizeof(u8)))
> net/dsa/dsa.c:          cd->rtable = kmalloc(pd->nr_chips * sizeof(s8),
> GFP_KERNEL);
> net/dsa/dsa.c:          memset(cd->rtable, -1, pd->nr_chips * sizeof(s8));
>
>

Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[Joe Perches]

On Fri, 2014-11-14 at 20:02 +0100, Fabian Frederick wrote:
> On 14 November 2014 at 19:47 Joe Perches <joe@perches.com> wrote:
> > On Fri, 2014-11-14 at 19:36 +0100, Fabian Frederick wrote:
> > > kmalloc_array manages count*sizeof overflow.
> >
> > Fundamentally correct, but is this necessary or useful?
> > sizeof(s8) isn't often going to be anything other than 1.
> Absolutely, I thought it was a struct :)
> 
> There must be a reason for so many cases though ...

Some might be style symmetry for other sizeof(othertype)
uses in the same paths, but most of them are just overkill
or maybe lack of understanding.

s8 is char so by definition it has to be 1.

I doubt any of the code dates from PDP-8/TOPS-10 days.

Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[Fabian Frederick]

> On 14 November 2014 at 20:14 Joe Perches <joe@perches.com> wrote:
>
>
> On Fri, 2014-11-14 at 20:02 +0100, Fabian Frederick wrote:
> > On 14 November 2014 at 19:47 Joe Perches <joe@perches.com> wrote:
> > > On Fri, 2014-11-14 at 19:36 +0100, Fabian Frederick wrote:
> > > > kmalloc_array manages count*sizeof overflow.
> > >
> > > Fundamentally correct, but is this necessary or useful?
> > > sizeof(s8) isn't often going to be anything other than 1.
> > Absolutely, I thought it was a struct :)
> >
> > There must be a reason for so many cases though ...
>
> Some might be style symmetry for other sizeof(othertype)
> uses in the same paths, but most of them are just overkill
> or maybe lack of understanding.
>
> s8 is char so by definition it has to be 1.
>
> I doubt any of the code dates from PDP-8/TOPS-10 days.
>
>
95% comes from drivers tree. I guess one patch to Greg would be enough.
Are you interested in those patches or can I do them with some "Suggested-by" ?

Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[Joe Perches]

On Fri, 2014-11-14 at 20:32 +0100, Fabian Frederick wrote:
> On 14 November 2014 at 20:14 Joe Perches <joe@perches.com> wrote:
> > On Fri, 2014-11-14 at 20:02 +0100, Fabian Frederick wrote:
> > > On 14 November 2014 at 19:47 Joe Perches <joe@perches.com> wrote:
> > > > On Fri, 2014-11-14 at 19:36 +0100, Fabian Frederick wrote:
> > > > > kmalloc_array manages count*sizeof overflow.
> > > >
> > > > Fundamentally correct, but is this necessary or useful?
> > > > sizeof(s8) isn't often going to be anything other than 1.
> > > Absolutely, I thought it was a struct :)
> > >
> > > There must be a reason for so many cases though ...
> >
> > Some might be style symmetry for other sizeof(othertype)
> > uses in the same paths, but most of them are just overkill
> > or maybe lack of understanding.
> 95% comes from drivers tree. I guess one patch to Greg would be enough.

I think it'd be better to send patches through the
appropriate various maintainers

Likely just using the 2nd level directory would be
good enough

$ git grep -E --name-only "\*\s*sizeof\s*\(\s*[us]8\s*\)" | \
  cut -f1-2 -d"/" | uniq
arch/arm
drivers/acpi
drivers/char
drivers/gpu
drivers/iio
drivers/infiniband
drivers/input
drivers/md
drivers/media
drivers/mtd
drivers/net
drivers/power
drivers/rtc
drivers/thermal
fs/compat_ioctl.c
net/dsa

> Are you interested in those patches or can I do them with some "Suggested-by" ?

Not really and no need.

cheers, Joe

Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array

[David Miller]

From: Fabian Frederick <fabf@skynet.be>
Date: Fri, 14 Nov 2014 19:36:42 +0100

> kmalloc_array manages count*sizeof overflow.
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied, and yes I read Joe's feedback and the rest of this thread
wrt. sizeof(u8/s8).  That can be done as a followup.