Re: [PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On Mon, Oct 07, 2013 at 01:34:44PM +0200, Erik Faye-Lund wrote:
> On Mon, Oct 7, 2013 at 10:34 AM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Rework the address table code for the host1x firewall. The previous
> > implementation allocated a bitfield but didn't check for a valid pointer
> > so it could potentially crash.
> 
> I don't think it could crash. The bitmaps was allocated as a 256-bit
> field, and the register offset gets AND'ed with 0xFF before being
> looked up. What am I missing?

The pointer returned from the allocation is never checked, so it could
theoretically be NULL and be used regardless.

Also I'm not sure that AND'ing with 0xff is the right thing to do here.

> > Furthermore setting up a bitfield makes
> > the code more complex than it needs to be.
> 
> Doesn't this perform worse than the current implementation? Going from
> 1 to 13 checks per write sounds less than ideal to me...

I'm not so sure. Caching should alleviate the issue with the increased
amount of data. Then there's the fact that previously we needed to
divide and compute the remainder of the division for the BIT_MASK and
BIT_WORD operations.

One other added benefit of this approach is that the address registers
are stored in a const array and therefore could reside in a read-only
memory region. With the previous approach, once somebody had access to
the bitmap, it could easily be overwritten with zeros and effectively
make the firewall useless. I'm not sure how likely that would be, but it
could be done.

I guess one could actually go and run a benchmark against both versions
and balance the performance impact against the possible security
implications. But given that we don't really have any benchmarks that's
pretty hard to do.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]