| Message ID | d3204a5b4776553455c2cfb1def72f1dae0dba25.1716054403.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dma-buf/fence-array: Add flex array to struct dma_fence_array | expand |
On Sat, May 18, 2024 at 07:47:02PM +0200, Christophe JAILLET wrote: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. > > The "struct dma_fence_array" can be refactored to add a flex array in order > to have the "callback structures allocated behind the array" be more > explicit. > > Do so: > - makes the code more readable and safer. > - allows using __counted_by() for additional checks > - avoids some pointer arithmetic in dma_fence_array_enable_signaling() > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] > Link: https://github.com/KSPP/linux/issues/160 [2] > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Yes please! :) Reviewed-by: Kees Cook <keescook@chromium.org>
Am 18.05.24 um 19:47 schrieb Christophe JAILLET: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1][2]. > > The "struct dma_fence_array" can be refactored to add a flex array in order > to have the "callback structures allocated behind the array" be more > explicit. > > Do so: > - makes the code more readable and safer. > - allows using __counted_by() for additional checks > - avoids some pointer arithmetic in dma_fence_array_enable_signaling() > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] > Link: https://github.com/KSPP/linux/issues/160 [2] > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only. > > Also, I don't think that 'cb' is a great name and the associated kernel-doc > description could certainly be improved. > Any proposal welcomed :) Ah, yes. That was also on my TODO list for a very long time. > --- > drivers/dma-buf/dma-fence-array.c | 10 ++++------ > include/linux/dma-fence-array.h | 3 +++ > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index 9b3ce8948351..9c55afaca607 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -70,7 +70,7 @@ static void dma_fence_array_cb_func(struct dma_fence *f, > static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > { > struct dma_fence_array *array = to_dma_fence_array(fence); > - struct dma_fence_array_cb *cb = (void *)(&array[1]); > + struct dma_fence_array_cb *cb = array->cb; > unsigned i; > > for (i = 0; i < array->num_fences; ++i) { > @@ -168,22 +168,20 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > bool signal_on_any) > { > struct dma_fence_array *array; > - size_t size = sizeof(*array); > > WARN_ON(!num_fences || !fences); > > - /* Allocate the callback structures behind the array. */ > - size += num_fences * sizeof(struct dma_fence_array_cb); > - array = kzalloc(size, GFP_KERNEL); > + array = kzalloc(struct_size(array, cb, num_fences), GFP_KERNEL); > if (!array) > return NULL; > > + array->num_fences = num_fences; > + > spin_lock_init(&array->lock); > dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, > context, seqno); > init_irq_work(&array->work, irq_dma_fence_array_work); > > - array->num_fences = num_fences; > atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); > array->fences = fences; > > diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h > index ec7f25def392..a793f9d5c73b 100644 > --- a/include/linux/dma-fence-array.h > +++ b/include/linux/dma-fence-array.h > @@ -33,6 +33,7 @@ struct dma_fence_array_cb { > * @num_pending: fences in the array still pending > * @fences: array of the fences > * @work: internal irq_work function > + * @cb: array of callback helpers > */ > struct dma_fence_array { > struct dma_fence base; > @@ -43,6 +44,8 @@ struct dma_fence_array { > struct dma_fence **fences; > > struct irq_work work; > + > + struct dma_fence_array_cb cb[] __counted_by(num_fences); Please name that callbacks, apart from that looks good to me. Regards, Christian. > }; > > /**
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 9b3ce8948351..9c55afaca607 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -70,7 +70,7 @@ static void dma_fence_array_cb_func(struct dma_fence *f, static bool dma_fence_array_enable_signaling(struct dma_fence *fence) { struct dma_fence_array *array = to_dma_fence_array(fence); - struct dma_fence_array_cb *cb = (void *)(&array[1]); + struct dma_fence_array_cb *cb = array->cb; unsigned i; for (i = 0; i < array->num_fences; ++i) { @@ -168,22 +168,20 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, bool signal_on_any) { struct dma_fence_array *array; - size_t size = sizeof(*array); WARN_ON(!num_fences || !fences); - /* Allocate the callback structures behind the array. */ - size += num_fences * sizeof(struct dma_fence_array_cb); - array = kzalloc(size, GFP_KERNEL); + array = kzalloc(struct_size(array, cb, num_fences), GFP_KERNEL); if (!array) return NULL; + array->num_fences = num_fences; + spin_lock_init(&array->lock); dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, context, seqno); init_irq_work(&array->work, irq_dma_fence_array_work); - array->num_fences = num_fences; atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); array->fences = fences; diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h index ec7f25def392..a793f9d5c73b 100644 --- a/include/linux/dma-fence-array.h +++ b/include/linux/dma-fence-array.h @@ -33,6 +33,7 @@ struct dma_fence_array_cb { * @num_pending: fences in the array still pending * @fences: array of the fences * @work: internal irq_work function + * @cb: array of callback helpers */ struct dma_fence_array { struct dma_fence base; @@ -43,6 +44,8 @@ struct dma_fence_array { struct dma_fence **fences; struct irq_work work; + + struct dma_fence_array_cb cb[] __counted_by(num_fences); }; /**
This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1][2]. The "struct dma_fence_array" can be refactored to add a flex array in order to have the "callback structures allocated behind the array" be more explicit. Do so: - makes the code more readable and safer. - allows using __counted_by() for additional checks - avoids some pointer arithmetic in dma_fence_array_enable_signaling() Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Link: https://github.com/KSPP/linux/issues/160 [2] Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only. Also, I don't think that 'cb' is a great name and the associated kernel-doc description could certainly be improved. Any proposal welcomed :) --- drivers/dma-buf/dma-fence-array.c | 10 ++++------ include/linux/dma-fence-array.h | 3 +++ 2 files changed, 7 insertions(+), 6 deletions(-)