| Message ID | 20210824173028.7528-2-alyssa.rosenzweig@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Bug fixes for lock_region | expand |
On 24/08/2021 18:30, Alyssa Rosenzweig wrote: > In lock_region, simplify the calculation of the region_width parameter. > This field is the size, but encoded as ceil(log2(size)) - 1. > ceil(log2(size)) may be computed directly as fls(size - 1). However, we > want to use the 64-bit versions as the amount to lock can exceed > 32-bits. > > This avoids undefined (and completely wrong) behaviour when locking all > memory (size ~0). In this case, the old code would "round up" ~0 to the > nearest page, overflowing to 0. Since fls(0) == 0, this would calculate > a region width of 10 + 0 = 10. But then the code would shift by > (region_width - 11) = -1. As shifting by a negative number is undefined, > UBSAN flags the bug. Of course, even if it were defined the behaviour is > wrong, instead of locking all memory almost none would get locked. > > The new form of the calculation corrects this special case and avoids > the undefined behaviour. > > Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> > Reported-and-tested-by: Chris Morgan <macromorgan@hotmail.com> > Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver") > Cc: <stable@vger.kernel.org> Reviewed-by: Steven Price <steven.price@arm.com> > --- > drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 0da5b3100ab1..f6e02d0392f4 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -62,21 +62,12 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr, > { > u8 region_width; > u64 region = iova & PAGE_MASK; > - /* > - * fls returns: > - * 1 .. 32 > - * > - * 10 + fls(num_pages) > - * results in the range (11 .. 42) > - */ > - > - size = round_up(size, PAGE_SIZE); > > - region_width = 10 + fls(size >> PAGE_SHIFT); > - if ((size >> PAGE_SHIFT) != (1ul << (region_width - 11))) { > - /* not pow2, so must go up to the next pow2 */ > - region_width += 1; > - } > + /* The size is encoded as ceil(log2) minus(1), which may be calculated > + * with fls. The size must be clamped to hardware bounds. > + */ > + size = max_t(u64, size, PAGE_SIZE); > + region_width = fls64(size - 1) - 1; > region |= region_width; > > /* Lock the region that needs to be updated */ >
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 0da5b3100ab1..f6e02d0392f4 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -62,21 +62,12 @@ static void lock_region(struct panfrost_device *pfdev, u32 as_nr, { u8 region_width; u64 region = iova & PAGE_MASK; - /* - * fls returns: - * 1 .. 32 - * - * 10 + fls(num_pages) - * results in the range (11 .. 42) - */ - - size = round_up(size, PAGE_SIZE); - region_width = 10 + fls(size >> PAGE_SHIFT); - if ((size >> PAGE_SHIFT) != (1ul << (region_width - 11))) { - /* not pow2, so must go up to the next pow2 */ - region_width += 1; - } + /* The size is encoded as ceil(log2) minus(1), which may be calculated + * with fls. The size must be clamped to hardware bounds. + */ + size = max_t(u64, size, PAGE_SIZE); + region_width = fls64(size - 1) - 1; region |= region_width; /* Lock the region that needs to be updated */
In lock_region, simplify the calculation of the region_width parameter. This field is the size, but encoded as ceil(log2(size)) - 1. ceil(log2(size)) may be computed directly as fls(size - 1). However, we want to use the 64-bit versions as the amount to lock can exceed 32-bits. This avoids undefined (and completely wrong) behaviour when locking all memory (size ~0). In this case, the old code would "round up" ~0 to the nearest page, overflowing to 0. Since fls(0) == 0, this would calculate a region width of 10 + 0 = 10. But then the code would shift by (region_width - 11) = -1. As shifting by a negative number is undefined, UBSAN flags the bug. Of course, even if it were defined the behaviour is wrong, instead of locking all memory almost none would get locked. The new form of the calculation corrects this special case and avoids the undefined behaviour. Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com> Reported-and-tested-by: Chris Morgan <macromorgan@hotmail.com> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver") Cc: <stable@vger.kernel.org> --- drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)