| Message ID | 20231214120824.655946-1-mika.kuoppala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] dma-buf: Fix dma reservation with zero fences | expand |
Am 14.12.23 um 13:08 schrieb Mika Kuoppala: > Driver can initialize without any fences. If so > roundup_power_of_two will overflow as it will try to > subtract one from initial value before shift, > (1 << fls_long(-1)). Ah, yes that reminds me that I wanted to take care of this as well. But solving it like this is the wrong approach. A couple of driver calculate the number of fences needed based on userspace input. If that results in zero then you certainly have a bug in your driver. Since calling dma_resv_reserve_fences() with num_fences==0 does make much sense we should really just warn about it and just return early from the function. Regards, Christian. > > Fix this using default (4) if num_fences is zero. > > Another more radical option would be to return error on > zero but that would need a callsite comb. > > Caught-by: UBSAN > Cc: Christian König <christian.koenig@amd.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/dma-buf/dma-resv.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 38b4110378de..f5ad3ecd0d4f 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -192,7 +192,10 @@ int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) > return 0; > max = max(old->num_fences + num_fences, old->max_fences * 2); > } else { > - max = max(4ul, roundup_pow_of_two(num_fences)); > + if (num_fences) > + max = max(4ul, roundup_pow_of_two(num_fences)); > + else > + max = 4ul; > } > > new = dma_resv_list_alloc(max);
Christian König <christian.koenig@amd.com> writes: > Am 14.12.23 um 13:08 schrieb Mika Kuoppala: >> Driver can initialize without any fences. If so >> roundup_power_of_two will overflow as it will try to >> subtract one from initial value before shift, >> (1 << fls_long(-1)). > > Ah, yes that reminds me that I wanted to take care of this as well. > > But solving it like this is the wrong approach. A couple of driver > calculate the number of fences needed based on userspace input. If that > results in zero then you certainly have a bug in your driver. > > Since calling dma_resv_reserve_fences() with num_fences==0 does make > much sense we should really just warn about it and just return early > from the function. Sounds like a plan. I will fix our driver to just omit the call if no fences (yet). Thanks, -Mika > Regards, > Christian. > > >> >> Fix this using default (4) if num_fences is zero. >> >> Another more radical option would be to return error on >> zero but that would need a callsite comb. >> >> Caught-by: UBSAN >> Cc: Christian König <christian.koenig@amd.com> >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> --- >> drivers/dma-buf/dma-resv.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >> index 38b4110378de..f5ad3ecd0d4f 100644 >> --- a/drivers/dma-buf/dma-resv.c >> +++ b/drivers/dma-buf/dma-resv.c >> @@ -192,7 +192,10 @@ int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) >> return 0; >> max = max(old->num_fences + num_fences, old->max_fences * 2); >> } else { >> - max = max(4ul, roundup_pow_of_two(num_fences)); >> + if (num_fences) >> + max = max(4ul, roundup_pow_of_two(num_fences)); >> + else >> + max = 4ul; >> } >> >> new = dma_resv_list_alloc(max);
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 38b4110378de..f5ad3ecd0d4f 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -192,7 +192,10 @@ int dma_resv_reserve_fences(struct dma_resv *obj, unsigned int num_fences) return 0; max = max(old->num_fences + num_fences, old->max_fences * 2); } else { - max = max(4ul, roundup_pow_of_two(num_fences)); + if (num_fences) + max = max(4ul, roundup_pow_of_two(num_fences)); + else + max = 4ul; } new = dma_resv_list_alloc(max);
Driver can initialize without any fences. If so roundup_power_of_two will overflow as it will try to subtract one from initial value before shift, (1 << fls_long(-1)). Fix this using default (4) if num_fences is zero. Another more radical option would be to return error on zero but that would need a callsite comb. Caught-by: UBSAN Cc: Christian König <christian.koenig@amd.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/dma-buf/dma-resv.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)