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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Mon, Oct 7, 2013 at 2:14 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> 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@xxxxxxxxx> 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.
>

Right. Thanks for clarifying.

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

Well, maybe not. I'm not entirely convinced it's *not*, though.

>> > 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.

That's an AND and a shift, not division and modulo, both single-cycle
instructions on ARM. I'm pretty sure that'd be a win.

> 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.

Perhaps the bitmap could be generated compile-time and stuck in
read-only memory?

> 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.

Well, perhaps we could have both? :)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux