| Message ID | 1415990202-28673-1-git-send-email-fabf@skynet.be |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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)); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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)); > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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" ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)