Re: PATCH: further kernel malloc - mallocarray
Question, comment and a potential bug ...
On Wed, Jul 16, 2014 at 04:54:49AM +, Doug Hogan wrote:
===
RCS file: /cvs/src/sys/arch/amd64/amd64/est.c,v
retrieving revision 1.33
diff -u -p -d -r1.33 est.c
--- sys/arch/amd64/amd64/est.c12 Jul 2014 18:44:41 - 1.33
+++ sys/arch/amd64/amd64/est.c15 Jul 2014 23:48:21 -
[...]
@@ -381,8 +381,8 @@ est_init(struct cpu_info *ci)
}
- if ((fake_table = malloc(sizeof(struct est_op) * 3, M_DEVBUF,
- M_NOWAIT)) == NULL) {
+ if ((fake_table = mallocarray(3, sizeof(struct est_op),
+ M_DEVBUF, M_NOWAIT)) == NULL) {
For obvious cases such as this, is it worth converting?
This is almost suggesting following conversion:
- ptr = malloc(sizeof(struct mystruct), ...);
+ ptr = mallocarray(1, sizeof(struct mystruct), ...);
===
RCS file: /cvs/src/sys/arch/i386/i386/est.c,v
retrieving revision 1.43
diff -u -p -d -r1.43 est.c
--- sys/arch/i386/i386/est.c 12 Jul 2014 18:44:41 - 1.43
+++ sys/arch/i386/i386/est.c 15 Jul 2014 23:48:23 -
[...]
@@ -1140,8 +1140,8 @@ est_init(struct cpu_info *ci, int vendor
}
- if ((fake_table = malloc(sizeof(struct est_op) * 3, M_DEVBUF,
- M_NOWAIT)) == NULL) {
+ if ((fake_table = mallocarray(3, sizeof(struct est_op),
+ M_DEVBUF, M_NOWAIT)) == NULL) {
[...]
===
RCS file: /cvs/src/sys/arch/sparc/dev/obio.c,v
retrieving revision 1.22
diff -u -p -d -r1.22 obio.c
--- sys/arch/sparc/dev/obio.c 5 Sep 2010 18:10:10 - 1.22
+++ sys/arch/sparc/dev/obio.c 15 Jul 2014 23:48:26 -
@@ -310,7 +310,7 @@ vmesattach(parent, self, args)
printf(\n);
if (vmeints == NULL) {
- vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
+ vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
M_NOWAIT | M_ZERO);
if (vmeints == NULL)
panic(vmesattach: can't allocate intrhand);
@@ -332,7 +332,7 @@ vmelattach(parent, self, args)
printf(\n);
if (vmeints == NULL) {
- vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
+ vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
M_NOWAIT | M_ZERO);
if (vmeints == NULL)
panic(vmelattach: can't allocate intrhand);
[...]
Index: sys/arch/sparc64/dev/vdsk.c
===
RCS file: /cvs/src/sys/arch/sparc64/dev/vdsk.c,v
retrieving revision 1.39
diff -u -p -d -r1.39 vdsk.c
--- sys/arch/sparc64/dev/vdsk.c 12 Jul 2014 18:44:43 - 1.39
+++ sys/arch/sparc64/dev/vdsk.c 15 Jul 2014 23:48:27 -
@@ -298,7 +298,7 @@ vdsk_attach(struct device *parent, struc
printf(, can't allocate dring\n);
goto free_map;
}
- sc-sc_vsd = malloc(32 * sizeof(*sc-sc_vsd), M_DEVBUF, M_NOWAIT);
+ sc-sc_vsd = mallocarray(32, sizeof(*sc-sc_vsd), M_DEVBUF, M_NOWAIT);
if (sc-sc_vsd == NULL) {
printf(, can't allocate software ring\n);
goto free_dring;
[...]
Index: sys/arch/sparc64/dev/vnet.c
===
RCS file: /cvs/src/sys/arch/sparc64/dev/vnet.c,v
retrieving revision 1.33
diff -u -p -d -r1.33 vnet.c
--- sys/arch/sparc64/dev/vnet.c 12 Jul 2014 18:44:43 - 1.33
+++ sys/arch/sparc64/dev/vnet.c 15 Jul 2014 23:48:27 -
@@ -1381,7 +1381,7 @@ vnet_init(struct ifnet *ifp)
sc-sc_vd = vnet_dring_alloc(sc-sc_dmatag, 128);
if (sc-sc_vd == NULL)
return;
- sc-sc_vsd = malloc(128 * sizeof(*sc-sc_vsd), M_DEVBUF, M_NOWAIT);
+ sc-sc_vsd = mallocarray(128, sizeof(*sc-sc_vsd), M_DEVBUF, M_NOWAIT);
if (sc-sc_vsd == NULL)
return;
[...]
Index: sys/dev/usb/uaudio.c
===
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.104
diff -u -p -d -r1.104 uaudio.c
--- sys/dev/usb/uaudio.c 12 Jul 2014 18:48:52 - 1.104
+++ sys/dev/usb/uaudio.c 15 Jul 2014 23:48:35 -
[...]
@@ -1962,7 +1962,8 @@ uaudio_identify_ac(struct uaudio_softc *
ibufend = ibuf + aclen;
dp = (const usb_descriptor_t *)ibuf;
ndps = 0;
- iot = malloc(sizeof(struct io_terminal) * 256, M_TEMP, M_NOWAIT |
M_ZERO);
+ iot = mallocarray(256, sizeof(struct io_terminal), M_TEMP,
+ M_NOWAIT | M_ZERO);
if (iot == NULL) {
printf(%s: no memory\n, __func__);
return USBD_NOMEM;
[...]
Index:
Re: PATCH: further kernel malloc - mallocarray
For the cases where it's more than just nitems * sizeof(item),
maybe it wouldn't be a bad idea to have something like:
static __inline int
MULT_OVERFLOWS(int x, int y)
{
const intmax_t max = 1UL sizeof(size_t) * 4;
return ((x = max || y = max) x 0 SIZE_MAX / x y);
}
(or maybe a macro version) in some public header someplace,
and associated assertions it where applicable.
Index: sys/dev/ic/mfi.c
- sc-sc_bbu_status = malloc(sizeof(*sc-sc_bbu_status) *
- sizeof(mfi_bbu_indicators), M_DEVBUF, M_WAITOK | M_ZERO);
+ sc-sc_bbu_status = mallocarray(sizeof(mfi_bbu_indicators),
+ sizeof(*sc-sc_bbu_status), M_DEVBUF, M_WAITOK | M_ZERO);
If we're not converting (numeric constant) * sizeof(foo) because it's
cheaper not to and realistically impossible to overflow anyway, then
I think we shouldn't convert sizeof() * sizeof() for the same reason.
Tedu:
- shellargp = malloc(4 * sizeof(char *), M_EXEC, M_WAITOK);
+ shellargp = mallocarray(4, sizeof(char *), M_EXEC, M_WAITOK);
Theo:
As for the final diff, I've been giving up on the known constant
scenario. It seems expensive.
Tedu:
Meh. :) I think they can be changed back if necessary; in the mean
time it makes it easier to see what's done and what remains.
Theo:
It is an extra register window on sparc and sparc64.
Re: PATCH: further kernel malloc - mallocarray
On Tue, Jul 15, 2014 at 11:34:01PM -0700, patrick keshishian wrote:
For obvious cases such as this, is it worth converting?
Maybe not. I left it since it is an array.
might be safer to change this (in a separate diff) to:
dc-dc_bs = mallocarray(ri-ri_rows,
ri-ri_cols * sizeof(struct wsdisplay_charcell),
...
That was a mistake. You're right.
Is the bit of `sizeof(mfi_bbu_indicators)' a bug in
the original source code?
should it not be `nitems(mfi_bbu_indicators)'?
I forgot to mention that. I think it's a bug since
mfi_bbu_indicators is defined as:
static const char *mfi_bbu_indicators[] = {
...
};
Re: PATCH: further kernel malloc - mallocarray
On Wed, Jul 16, 2014 at 04:54:49AM +, Doug Hogan wrote:
+ if ((fake_table = mallocarray(3, sizeof(struct est_op),
It's not necessary to use mallocarray() for well known constants.
Few examples below.
--- sys/arch/i386/i386/est.c 12 Jul 2014 18:44:41 - 1.43
+++ sys/arch/i386/i386/est.c 15 Jul 2014 23:48:23 -
...
- if ((fake_table = malloc(sizeof(struct est_op) * 3, M_DEVBUF,
^^^
diff -u -p -d -r1.31 if_tsec.c
--- sys/arch/socppc/dev/if_tsec.c 12 Jul 2014 18:44:42 - 1.31
+++ sys/arch/socppc/dev/if_tsec.c 15 Jul 2014 23:48:25 -
@@ -909,7 +909,7 @@ tsec_up(struct tsec_softc *sc)
TSEC_NTXDESC * sizeof(struct tsec_desc), 8);
sc-sc_txdesc = TSEC_DMA_KVA(sc-sc_txring);
- sc-sc_txbuf = malloc(sizeof(struct tsec_buf) * TSEC_NTXDESC,
+ sc-sc_txbuf = mallocarray(TSEC_NTXDESC, sizeof(struct tsec_buf),
M_DEVBUF, M_WAITOK);
for (i = 0; i TSEC_NTXDESC; i++) {
txb = sc-sc_txbuf[i];
@@ -935,7 +935,7 @@ tsec_up(struct tsec_softc *sc)
TSEC_NRXDESC * sizeof(struct tsec_desc), 8);
sc-sc_rxdesc = TSEC_DMA_KVA(sc-sc_rxring);
- sc-sc_rxbuf = malloc(sizeof(struct tsec_buf) * TSEC_NRXDESC,
+ sc-sc_rxbuf = mallocarray(TSEC_NRXDESC, sizeof(struct tsec_buf),
M_DEVBUF, M_WAITOK);
for (i = 0; i TSEC_NRXDESC; i++) {
Index: sys/arch/sparc/dev/obio.c
===
RCS file: /cvs/src/sys/arch/sparc/dev/obio.c,v
retrieving revision 1.22
diff -u -p -d -r1.22 obio.c
--- sys/arch/sparc/dev/obio.c 5 Sep 2010 18:10:10 - 1.22
+++ sys/arch/sparc/dev/obio.c 15 Jul 2014 23:48:26 -
@@ -310,7 +310,7 @@ vmesattach(parent, self, args)
printf(\n);
if (vmeints == NULL) {
- vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
+ vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
^^^
M_NOWAIT | M_ZERO);
if (vmeints == NULL)
panic(vmesattach: can't allocate intrhand);
@@ -332,7 +332,7 @@ vmelattach(parent, self, args)
printf(\n);
if (vmeints == NULL) {
- vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
+ vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
^^^
M_NOWAIT | M_ZERO);
if (vmeints == NULL)
panic(vmelattach: can't allocate intrhand);
Index: sys/arch/sparc/dev/xd.c
===
RCS file: /cvs/src/sys/arch/sparc/dev/xd.c,v
retrieving revision 1.62
diff -u -p -d -r1.62 xd.c
--- sys/arch/sparc/dev/xd.c 11 Jul 2014 16:35:40 - 1.62
+++ sys/arch/sparc/dev/xd.c 15 Jul 2014 23:48:26 -
@@ -414,7 +414,7 @@ xdcattach(parent, self, aux)
/* Setup device view of DVMA address */
xdc-dvmaiopb = (struct xd_iopb *) ((u_long) xdc-iopbase - DVMA_BASE);
- xdc-reqs = malloc(XDC_MAXIOPB * sizeof(struct xd_iorq), M_DEVBUF,
+ xdc-reqs = mallocarray(XDC_MAXIOPB, sizeof(struct xd_iorq), M_DEVBUF,
^^^
M_NOWAIT | M_ZERO);
if (xdc-reqs == NULL)
panic(xdc malloc);
Index: sys/arch/sparc/dev/xy.c
===
RCS file: /cvs/src/sys/arch/sparc/dev/xy.c,v
retrieving revision 1.59
diff -u -p -d -r1.59 xy.c
--- sys/arch/sparc/dev/xy.c 11 Jul 2014 16:35:40 - 1.59
+++ sys/arch/sparc/dev/xy.c 15 Jul 2014 23:48:26 -
@@ -364,7 +364,7 @@ xycattach(parent, self, aux)
xyc-iopbase = tmp;
xyc-iopbase = dtmp; /* XXX TMP HACK */
xyc-dvmaiopb = (struct xy_iopb *) ((u_long)dtmp - DVMA_BASE);
- xyc-reqs = malloc(XYC_MAXIOPB * sizeof(struct xy_iorq), M_DEVBUF,
+ xyc-reqs = mallocarray(XYC_MAXIOPB, sizeof(struct xy_iorq), M_DEVBUF,
^^^
M_NOWAIT | M_ZERO);
if (xyc-reqs == NULL)
panic(xyc malloc);
Index: sys/arch/sparc64/dev/vdsk.c
===
RCS file: /cvs/src/sys/arch/sparc64/dev/vdsk.c,v
retrieving revision 1.39
diff -u -p -d -r1.39 vdsk.c
--- sys/arch/sparc64/dev/vdsk.c 12 Jul 2014 18:44:43 - 1.39
+++ sys/arch/sparc64/dev/vdsk.c 15 Jul 2014 23:48:27 -
@@ -298,7 +298,7 @@ vdsk_attach(struct device *parent, struc
printf(, can't allocate dring\n);
goto free_map;
}
- sc-sc_vsd = malloc(32 * sizeof(*sc-sc_vsd), M_DEVBUF, M_NOWAIT);
+ sc-sc_vsd = mallocarray(32, sizeof(*sc-sc_vsd),
Re: PATCH: further kernel malloc - mallocarray
I would really really prefer if we can keep these as const*const
conversions instead of const, const.
We will see performance losses from doing this operation at runtime.
On Wed, Jul 16, 2014 at 04:54:49AM +, Doug Hogan wrote:
+ if ((fake_table = mallocarray(3, sizeof(struct est_op),
It's not necessary to use mallocarray() for well known constants.
Few examples below.
--- sys/arch/i386/i386/est.c12 Jul 2014 18:44:41 - 1.43
+++ sys/arch/i386/i386/est.c15 Jul 2014 23:48:23 -
...
- if ((fake_table = malloc(sizeof(struct est_op) * 3, M_DEVBUF,
^^^
diff -u -p -d -r1.31 if_tsec.c
--- sys/arch/socppc/dev/if_tsec.c 12 Jul 2014 18:44:42 - 1.31
+++ sys/arch/socppc/dev/if_tsec.c 15 Jul 2014 23:48:25 -
@@ -909,7 +909,7 @@ tsec_up(struct tsec_softc *sc)
TSEC_NTXDESC * sizeof(struct tsec_desc), 8);
sc-sc_txdesc = TSEC_DMA_KVA(sc-sc_txring);
- sc-sc_txbuf = malloc(sizeof(struct tsec_buf) * TSEC_NTXDESC,
+ sc-sc_txbuf = mallocarray(TSEC_NTXDESC, sizeof(struct tsec_buf),
M_DEVBUF, M_WAITOK);
for (i = 0; i TSEC_NTXDESC; i++) {
txb = sc-sc_txbuf[i];
@@ -935,7 +935,7 @@ tsec_up(struct tsec_softc *sc)
TSEC_NRXDESC * sizeof(struct tsec_desc), 8);
sc-sc_rxdesc = TSEC_DMA_KVA(sc-sc_rxring);
- sc-sc_rxbuf = malloc(sizeof(struct tsec_buf) * TSEC_NRXDESC,
+ sc-sc_rxbuf = mallocarray(TSEC_NRXDESC, sizeof(struct tsec_buf),
M_DEVBUF, M_WAITOK);
for (i = 0; i TSEC_NRXDESC; i++) {
Index: sys/arch/sparc/dev/obio.c
===
RCS file: /cvs/src/sys/arch/sparc/dev/obio.c,v
retrieving revision 1.22
diff -u -p -d -r1.22 obio.c
--- sys/arch/sparc/dev/obio.c 5 Sep 2010 18:10:10 - 1.22
+++ sys/arch/sparc/dev/obio.c 15 Jul 2014 23:48:26 -
@@ -310,7 +310,7 @@ vmesattach(parent, self, args)
printf(\n);
if (vmeints == NULL) {
- vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
+ vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
^^^
M_NOWAIT | M_ZERO);
if (vmeints == NULL)
panic(vmesattach: can't allocate intrhand);
@@ -332,7 +332,7 @@ vmelattach(parent, self, args)
printf(\n);
if (vmeints == NULL) {
- vmeints = malloc(256 * sizeof(struct intrhand *), M_TEMP,
+ vmeints = mallocarray(256, sizeof(struct intrhand *), M_TEMP,
^^^
M_NOWAIT | M_ZERO);
if (vmeints == NULL)
panic(vmelattach: can't allocate intrhand);
Index: sys/arch/sparc/dev/xd.c
===
RCS file: /cvs/src/sys/arch/sparc/dev/xd.c,v
retrieving revision 1.62
diff -u -p -d -r1.62 xd.c
--- sys/arch/sparc/dev/xd.c 11 Jul 2014 16:35:40 - 1.62
+++ sys/arch/sparc/dev/xd.c 15 Jul 2014 23:48:26 -
@@ -414,7 +414,7 @@ xdcattach(parent, self, aux)
/* Setup device view of DVMA address */
xdc-dvmaiopb = (struct xd_iopb *) ((u_long) xdc-iopbase - DVMA_BASE);
- xdc-reqs = malloc(XDC_MAXIOPB * sizeof(struct xd_iorq), M_DEVBUF,
+ xdc-reqs = mallocarray(XDC_MAXIOPB, sizeof(struct xd_iorq), M_DEVBUF,
^^^
M_NOWAIT | M_ZERO);
if (xdc-reqs == NULL)
panic(xdc malloc);
Index: sys/arch/sparc/dev/xy.c
===
RCS file: /cvs/src/sys/arch/sparc/dev/xy.c,v
retrieving revision 1.59
diff -u -p -d -r1.59 xy.c
--- sys/arch/sparc/dev/xy.c 11 Jul 2014 16:35:40 - 1.59
+++ sys/arch/sparc/dev/xy.c 15 Jul 2014 23:48:26 -
@@ -364,7 +364,7 @@ xycattach(parent, self, aux)
xyc-iopbase = tmp;
xyc-iopbase = dtmp; /* XXX TMP HACK */
xyc-dvmaiopb = (struct xy_iopb *) ((u_long)dtmp - DVMA_BASE);
- xyc-reqs = malloc(XYC_MAXIOPB * sizeof(struct xy_iorq), M_DEVBUF,
+ xyc-reqs = mallocarray(XYC_MAXIOPB, sizeof(struct xy_iorq), M_DEVBUF,
^^^
M_NOWAIT | M_ZERO);
if (xyc-reqs == NULL)
panic(xyc malloc);
Index: sys/arch/sparc64/dev/vdsk.c
===
RCS file: /cvs/src/sys/arch/sparc64/dev/vdsk.c,v
retrieving revision 1.39
diff -u -p -d -r1.39 vdsk.c
--- sys/arch/sparc64/dev/vdsk.c 12 Jul 2014 18:44:43 - 1.39
+++ sys/arch/sparc64/dev/vdsk.c 15 Jul 2014 23:48:27 -
@@ -298,7 +298,7 @@ vdsk_attach(struct device *parent, struc
printf(, can't
Re: PATCH: further kernel malloc - mallocarray
From: Theo de Raadt dera...@cvs.openbsd.org Date: Wed, 16 Jul 2014 08:18:34 -0600 I would really really prefer if we can keep these as const*const conversions instead of const, const. Indeed, conversion to mallocarray only makes sence if one of the multiplication operands is a variable.
Re: PATCH: further kernel malloc - mallocarray
From: Theo de Raadt dera...@cvs.openbsd.org Date: Wed, 16 Jul 2014 08:18:34 -0600 I would really really prefer if we can keep these as const*const conversions instead of const, const. Indeed, conversion to mallocarray only makes sence if one of the multiplication operands is a variable. That said, it is tricky. I've kept my eye out for one of the #define's to actually utilize a variable behind the scene. Haven't found one yet, but the potential is there.
Re: PATCH: further kernel malloc - mallocarray
static __inline int
MULT_OVERFLOWS(int x, int y)
{
const intmax_t max = 1UL sizeof(size_t) * 4;
return ((x = max || y = max) x 0 SIZE_MAX / x y);
}
(or maybe a macro version) in some public header someplace,
and associated assertions it where applicable.
The coding pattern currently chosen through a discussion by
Ted and myself is to convert:
l = n * s;
p = malloc(l, ...)
if (!p)
fail;
to either:
p = mallocarray(n, s, ...)
l = n * s;
if (!p)
fail;
or
p = mallocarray(n, s, ...)
if (!p)
fail;
l = n * s;
We think that is more clear than the addition of a add-on API
for integer overflow which people will avoid. The idea behind
mallocarray() is that it is in-your-face -- we want to develop
the mindset that any malloc() gets looked at from the perspective
of int overflow right in it's arguments.
