| Message ID | 20201027121725.24660-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | slab: provide and use krealloc_array() | expand |
Am 27.10.20 um 13:17 schrieb Bartosz Golaszewski: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Use the helper that checks for overflows internally instead of manually > calculating the size of the new array. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Acked-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/sync_file.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 5a5a1da01a00..2925ea03eef0 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -270,8 +270,8 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > fences[i++] = dma_fence_get(a_fences[0]); > > if (num_fences > i) { > - nfences = krealloc(fences, i * sizeof(*fences), > - GFP_KERNEL); > + nfences = krealloc_array(fences, i, > + sizeof(*fences), GFP_KERNEL); > if (!nfences) > goto err; >
On Tue, Oct 27, 2020 at 03:29:11PM +0200, Andy Shevchenko wrote: > On Tue, Oct 27, 2020 at 01:17:21PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Use the helper that checks for overflows internally instead of manually > > calculating the size of the new array. After addressing a nit in previous message: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Use the helper that checks for overflows internally instead of manually > calculating the size of the new array. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> No problem with the patch, it does introduce some symmetry in the code. Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/vhost/vringh.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > index 8bd8b403f087..08a0e1c842df 100644 > --- a/drivers/vhost/vringh.c > +++ b/drivers/vhost/vringh.c > @@ -198,7 +198,8 @@ static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp) > > flag = (iov->max_num & VRINGH_IOV_ALLOCATED); > if (flag) > - new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp); > + new = krealloc_array(iov->iov, new_num, > + sizeof(struct iovec), gfp); > else { > new = kmalloc_array(new_num, sizeof(struct iovec), gfp); > if (new) { > -- > 2.29.1
On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote: > On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Use the helper that checks for overflows internally instead of manually > > calculating the size of the new array. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > No problem with the patch, it does introduce some symmetry in the code. Perhaps more symmetry by using kmemdup --- drivers/vhost/vringh.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c index 8bd8b403f087..99222a3651cd 100644 --- a/drivers/vhost/vringh.c +++ b/drivers/vhost/vringh.c @@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh, static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp) { struct kvec *new; - unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; + size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; + size_t size; if (new_num < 8) new_num = 8; - flag = (iov->max_num & VRINGH_IOV_ALLOCATED); - if (flag) - new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp); - else { - new = kmalloc_array(new_num, sizeof(struct iovec), gfp); - if (new) { - memcpy(new, iov->iov, - iov->max_num * sizeof(struct iovec)); - flag = VRINGH_IOV_ALLOCATED; - } - } + if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec), &size))) + return -ENOMEM; + + if (iov->max_num & VRINGH_IOV_ALLOCATED) + new = krealloc(iov->iov, size, gfp); + else + new = kmemdup(iov->iov, size, gfp); if (!new) return -ENOMEM; iov->iov = new; - iov->max_num = (new_num | flag); + iov->max_num = new_num | VRINGH_IOV_ALLOCATED; return 0; }
On Tue, Oct 27, 2020 at 5:50 PM Joe Perches <joe@perches.com> wrote: > > On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote: > > On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > Use the helper that checks for overflows internally instead of manually > > > calculating the size of the new array. > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > No problem with the patch, it does introduce some symmetry in the code. > > Perhaps more symmetry by using kmemdup > --- > drivers/vhost/vringh.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > index 8bd8b403f087..99222a3651cd 100644 > --- a/drivers/vhost/vringh.c > +++ b/drivers/vhost/vringh.c > @@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh, > static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp) > { > struct kvec *new; > - unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; > + size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; > + size_t size; > > if (new_num < 8) > new_num = 8; > > - flag = (iov->max_num & VRINGH_IOV_ALLOCATED); > - if (flag) > - new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp); > - else { > - new = kmalloc_array(new_num, sizeof(struct iovec), gfp); > - if (new) { > - memcpy(new, iov->iov, > - iov->max_num * sizeof(struct iovec)); > - flag = VRINGH_IOV_ALLOCATED; > - } > - } > + if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec), &size))) > + return -ENOMEM; > + The whole point of using helpers such as kmalloc_array() is not doing these checks manually. Bartosz > + if (iov->max_num & VRINGH_IOV_ALLOCATED) > + new = krealloc(iov->iov, size, gfp); > + else > + new = kmemdup(iov->iov, size, gfp); > if (!new) > return -ENOMEM; > iov->iov = new; > - iov->max_num = (new_num | flag); > + iov->max_num = new_num | VRINGH_IOV_ALLOCATED; > return 0; > } > > >
On Tue, 2020-10-27 at 17:58 +0100, Bartosz Golaszewski wrote: > On Tue, Oct 27, 2020 at 5:50 PM Joe Perches <joe@perches.com> wrote: > > > > On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote: > > > On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > Use the helper that checks for overflows internally instead of manually > > > > calculating the size of the new array. > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > No problem with the patch, it does introduce some symmetry in the code. > > > > Perhaps more symmetry by using kmemdup > > --- > > drivers/vhost/vringh.c | 23 ++++++++++------------- > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > index 8bd8b403f087..99222a3651cd 100644 > > --- a/drivers/vhost/vringh.c > > +++ b/drivers/vhost/vringh.c > > @@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh, > > static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp) > > { > > struct kvec *new; > > - unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; > > + size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; > > + size_t size; > > > > if (new_num < 8) > > new_num = 8; > > > > - flag = (iov->max_num & VRINGH_IOV_ALLOCATED); > > - if (flag) > > - new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp); > > - else { > > - new = kmalloc_array(new_num, sizeof(struct iovec), gfp); > > - if (new) { > > - memcpy(new, iov->iov, > > - iov->max_num * sizeof(struct iovec)); > > - flag = VRINGH_IOV_ALLOCATED; > > - } > > - } > > + if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec), &size))) > > + return -ENOMEM; > > + > > The whole point of using helpers such as kmalloc_array() is not doing > these checks manually. Tradeoffs for in readability for overflow and not mistyping or doing the multiplication of iov->max_num * sizeof(struct iovec) twice. Just fyi: the realloc doesn't do a multiplication overflow test as written so the suggestion is slightly more resistant to defect.
On Tue, Oct 27, 2020 at 6:08 PM Joe Perches <joe@perches.com> wrote: > > On Tue, 2020-10-27 at 17:58 +0100, Bartosz Golaszewski wrote: > > On Tue, Oct 27, 2020 at 5:50 PM Joe Perches <joe@perches.com> wrote: > > > > > > On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote: > > > > On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > > > Use the helper that checks for overflows internally instead of manually > > > > > calculating the size of the new array. > > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > No problem with the patch, it does introduce some symmetry in the code. > > > > > > Perhaps more symmetry by using kmemdup > > > --- > > > drivers/vhost/vringh.c | 23 ++++++++++------------- > > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > > index 8bd8b403f087..99222a3651cd 100644 > > > --- a/drivers/vhost/vringh.c > > > +++ b/drivers/vhost/vringh.c > > > @@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh, > > > static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp) > > > { > > > struct kvec *new; > > > - unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; > > > + size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2; > > > + size_t size; > > > > > > if (new_num < 8) > > > new_num = 8; > > > > > > - flag = (iov->max_num & VRINGH_IOV_ALLOCATED); > > > - if (flag) > > > - new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp); > > > - else { > > > - new = kmalloc_array(new_num, sizeof(struct iovec), gfp); > > > - if (new) { > > > - memcpy(new, iov->iov, > > > - iov->max_num * sizeof(struct iovec)); > > > - flag = VRINGH_IOV_ALLOCATED; > > > - } > > > - } > > > + if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec), &size))) > > > + return -ENOMEM; > > > + > > > > The whole point of using helpers such as kmalloc_array() is not doing > > these checks manually. > > Tradeoffs for in readability for overflow and not mistyping or doing > the multiplication of iov->max_num * sizeof(struct iovec) twice. > It's out of scope for this series - I want to add users for krealloc_array(), not refactor code I don't really know. If the maintainer of this bit objects, it can be dropped. > Just fyi: > > the realloc doesn't do a multiplication overflow test as written so the > suggestion is slightly more resistant to defect. > I'm not sure what your point is. I used krealloc_array() exactly for this reason - to add the overflow test. BTW I suppose kmalloc_array() here can be replaced with krealloc_array() if the original pointer is NULL the first time it's called. Bartosz
On Tue, Oct 27, 2020 at 01:17:22PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Use the helper that checks for overflows internally instead of manually > calculating the size of the new array. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/edac/ghes_edac.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c > index a918ca93e4f7..6d1ddecbf0da 100644 > --- a/drivers/edac/ghes_edac.c > +++ b/drivers/edac/ghes_edac.c > @@ -207,8 +207,8 @@ static void enumerate_dimms(const struct dmi_header *dh, void *arg) > if (!hw->num_dimms || !(hw->num_dimms % 16)) { > struct dimm_info *new; > > - new = krealloc(hw->dimms, (hw->num_dimms + 16) * sizeof(struct dimm_info), > - GFP_KERNEL); > + new = krealloc_array(hw->dimms, hw->num_dimms + 16, > + sizeof(struct dimm_info), GFP_KERNEL); > if (!new) { > WARN_ON_ONCE(1); > return; > -- Sure, why not. Acked-by: Borislav Petkov <bp@suse.de>
On Tue, Oct 27, 2020 at 1:17 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Andy brought to my attention the fact that users allocating an array of > equally sized elements should check if the size multiplication doesn't > overflow. This is why we have helpers like kmalloc_array(). > > However we don't have krealloc_array() equivalent and there are many > users who do their own multiplication when calling krealloc() for arrays. > > This series provides krealloc_array() and uses it in a couple places. > > A separate series will follow adding devm_krealloc_array() which is > needed in the xilinx adc driver. The series: Acked-by: Linus Walleij <linus.walleij@linaro.org> I really like this. Yours, Linus Walleij
From: Bartosz Golaszewski <bgolaszewski@baylibre.com> Andy brought to my attention the fact that users allocating an array of equally sized elements should check if the size multiplication doesn't overflow. This is why we have helpers like kmalloc_array(). However we don't have krealloc_array() equivalent and there are many users who do their own multiplication when calling krealloc() for arrays. This series provides krealloc_array() and uses it in a couple places. A separate series will follow adding devm_krealloc_array() which is needed in the xilinx adc driver. Bartosz Golaszewski (8): mm: slab: provide krealloc_array() ALSA: pcm: use krealloc_array() vhost: vringh: use krealloc_array() pinctrl: use krealloc_array() edac: ghes: use krealloc_array() drm: atomic: use krealloc_array() hwtracing: intel: use krealloc_array() dma-buf: use krealloc_array() drivers/dma-buf/sync_file.c | 4 ++-- drivers/edac/ghes_edac.c | 4 ++-- drivers/gpu/drm/drm_atomic.c | 3 ++- drivers/hwtracing/intel_th/msu.c | 2 +- drivers/pinctrl/pinctrl-utils.c | 2 +- drivers/vhost/vringh.c | 3 ++- include/linux/slab.h | 11 +++++++++++ sound/core/pcm_lib.c | 4 ++-- 8 files changed, 23 insertions(+), 10 deletions(-)