* [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array @ 2014-11-14 18:36 Fabian Frederick 2014-11-14 18:47 ` Joe Perches 2014-11-16 19:43 ` David Miller 0 siblings, 2 replies; 7+ messages in thread From: Fabian Frederick @ 2014-11-14 18:36 UTC (permalink / raw) To: linux-kernel; +Cc: Fabian Frederick, David S. Miller, netdev 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array 2014-11-14 18:36 [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array Fabian Frederick @ 2014-11-14 18:47 ` Joe Perches 2014-11-14 19:02 ` Fabian Frederick 2014-11-16 19:43 ` David Miller 1 sibling, 1 reply; 7+ messages in thread From: Joe Perches @ 2014-11-14 18:47 UTC (permalink / raw) To: Fabian Frederick; +Cc: linux-kernel, David S. Miller, netdev 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)); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array 2014-11-14 18:47 ` Joe Perches @ 2014-11-14 19:02 ` Fabian Frederick 2014-11-14 19:14 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Fabian Frederick @ 2014-11-14 19:02 UTC (permalink / raw) To: Joe Perches; +Cc: netdev, linux-kernel, David S. Miller > 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)); > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array 2014-11-14 19:02 ` Fabian Frederick @ 2014-11-14 19:14 ` Joe Perches 2014-11-14 19:32 ` Fabian Frederick 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2014-11-14 19:14 UTC (permalink / raw) To: Fabian Frederick; +Cc: netdev, linux-kernel, David S. Miller 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array 2014-11-14 19:14 ` Joe Perches @ 2014-11-14 19:32 ` Fabian Frederick 2014-11-14 19:42 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Fabian Frederick @ 2014-11-14 19:32 UTC (permalink / raw) To: Joe Perches; +Cc: netdev, linux-kernel, David S. Miller > 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" ? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array 2014-11-14 19:32 ` Fabian Frederick @ 2014-11-14 19:42 ` Joe Perches 0 siblings, 0 replies; 7+ messages in thread From: Joe Perches @ 2014-11-14 19:42 UTC (permalink / raw) To: Fabian Frederick; +Cc: netdev, linux-kernel, David S. Miller 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array 2014-11-14 18:36 [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array Fabian Frederick 2014-11-14 18:47 ` Joe Perches @ 2014-11-16 19:43 ` David Miller 1 sibling, 0 replies; 7+ messages in thread From: David Miller @ 2014-11-16 19:43 UTC (permalink / raw) To: fabf; +Cc: linux-kernel, netdev 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-11-16 19:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-14 18:36 [PATCH 1/1 net-next] net: dsa: replace count*size kmalloc by kmalloc_array Fabian Frederick 2014-11-14 18:47 ` Joe Perches 2014-11-14 19:02 ` Fabian Frederick 2014-11-14 19:14 ` Joe Perches 2014-11-14 19:32 ` Fabian Frederick 2014-11-14 19:42 ` Joe Perches 2014-11-16 19:43 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).