mbox series

[v2,0/4] drm/panfrost: Bug fixes for lock_region

Message ID 20210824173028.7528-1-alyssa.rosenzweig@collabora.com (mailing list archive)
Headers show
Series drm/panfrost: Bug fixes for lock_region | expand

Message

Alyssa Rosenzweig Aug. 24, 2021, 5:30 p.m. UTC
Chris Morgan reported UBSAN errors in panfrost and tracked them down to
the size computation in lock_region. This calculation is overcomplicated
(cargo culted from kbase) and can be simplified with kernel helpers and
some mathematical identities. The first patch in the series rewrites the
calculation in a form avoiding undefined behaviour; Chris confirms it
placates UBSAN.

While researching this function, I noticed a pair of other potential
bugs: Bifrost can lock more than 4GiB at a time, but must lock at least
32KiB at a time. The latter patches in the series handle these cases.

In review of v1 of this series, Steven pointed out a fourth potential
bug: rounding down the iova can truncate the lock region. v2 adds a new
patch for this case.

The size computation was unit-tested in userspace. Relevant code below,
just missing some copypaste definitions for fls64/clamp/etc:

	#define MIN_LOCK (1ULL << 12)
	#define MAX_LOCK (1ULL << 48)

	struct {
		uint64_t size;
		uint8_t encoded;
	} tests[] = {
		/* Clamping */
		{ 0, 11 },
		{ 1, 11 },
		{ 2, 11 },
		{ 4095, 11 },
		/* Power of two */
		{ 4096, 11 },
		/* Round up */
		{ 4097, 12 },
		{ 8192, 12 },
		{ 16384, 13 },
		{ 16385, 14 },
		/* Maximum */
		{ ~0ULL, 47 },
	};

	static uint8_t region_width(uint64_t size)
	{
		size = clamp(size, MIN_LOCK, MAX_LOCK);
		return fls64(size - 1) - 1;
	}

	int main(int argc, char **argv)
	{
		for (unsigned i = 0; i < ARRAY_SIZE(tests); ++i) {
			uint64_t test = tests[i].size;
			uint8_t expected = tests[i].encoded;
			uint8_t actual = region_width(test);

			assert(expected == actual);
		}
	}

Changes in v2:

* New patch for non-aligned lock addresses
* Commit message improvements.
* Add Steven's tags.

Alyssa Rosenzweig (4):
  drm/panfrost: Simplify lock_region calculation
  drm/panfrost: Use u64 for size in lock_region
  drm/panfrost: Clamp lock region to Bifrost minimum
  drm/panfrost: Handle non-aligned lock addresses

 drivers/gpu/drm/panfrost/panfrost_mmu.c  | 32 ++++++++++--------------
 drivers/gpu/drm/panfrost/panfrost_regs.h |  2 ++
 2 files changed, 15 insertions(+), 19 deletions(-)

Comments

Rob Herring (Arm) Aug. 24, 2021, 10:37 p.m. UTC | #1
On Tue, Aug 24, 2021 at 12:30 PM Alyssa Rosenzweig
<alyssa.rosenzweig@collabora.com> wrote:
>
> Chris Morgan reported UBSAN errors in panfrost and tracked them down to
> the size computation in lock_region. This calculation is overcomplicated
> (cargo culted from kbase) and can be simplified with kernel helpers and
> some mathematical identities. The first patch in the series rewrites the
> calculation in a form avoiding undefined behaviour; Chris confirms it
> placates UBSAN.
>
> While researching this function, I noticed a pair of other potential
> bugs: Bifrost can lock more than 4GiB at a time, but must lock at least
> 32KiB at a time. The latter patches in the series handle these cases.
>
> In review of v1 of this series, Steven pointed out a fourth potential
> bug: rounding down the iova can truncate the lock region. v2 adds a new
> patch for this case.
>
> The size computation was unit-tested in userspace. Relevant code below,
> just missing some copypaste definitions for fls64/clamp/etc:
>
>         #define MIN_LOCK (1ULL << 12)
>         #define MAX_LOCK (1ULL << 48)
>
>         struct {
>                 uint64_t size;
>                 uint8_t encoded;
>         } tests[] = {
>                 /* Clamping */
>                 { 0, 11 },
>                 { 1, 11 },
>                 { 2, 11 },
>                 { 4095, 11 },
>                 /* Power of two */
>                 { 4096, 11 },
>                 /* Round up */
>                 { 4097, 12 },
>                 { 8192, 12 },
>                 { 16384, 13 },
>                 { 16385, 14 },
>                 /* Maximum */
>                 { ~0ULL, 47 },
>         };
>
>         static uint8_t region_width(uint64_t size)
>         {
>                 size = clamp(size, MIN_LOCK, MAX_LOCK);
>                 return fls64(size - 1) - 1;
>         }
>
>         int main(int argc, char **argv)
>         {
>                 for (unsigned i = 0; i < ARRAY_SIZE(tests); ++i) {
>                         uint64_t test = tests[i].size;
>                         uint8_t expected = tests[i].encoded;
>                         uint8_t actual = region_width(test);
>
>                         assert(expected == actual);
>                 }
>         }
>
> Changes in v2:
>
> * New patch for non-aligned lock addresses
> * Commit message improvements.
> * Add Steven's tags.
>
> Alyssa Rosenzweig (4):
>   drm/panfrost: Simplify lock_region calculation
>   drm/panfrost: Use u64 for size in lock_region
>   drm/panfrost: Clamp lock region to Bifrost minimum
>   drm/panfrost: Handle non-aligned lock addresses
>
>  drivers/gpu/drm/panfrost/panfrost_mmu.c  | 32 ++++++++++--------------
>  drivers/gpu/drm/panfrost/panfrost_regs.h |  2 ++
>  2 files changed, 15 insertions(+), 19 deletions(-)

For the series,

Reviewed-by: Rob Herring <robh@kernel.org>