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.
Re: PATCH: further kernel malloc -> mallocarray
> > From: Theo de Raadt > > 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
> From: Theo de Raadt > 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
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
> >
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;
>
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
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
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);
> i
