On Tuesday, May 8, 2018 10:02:22 AM PDT Scott D Phillips wrote:
> Kenneth Graunke <kenn...@whitecape.org> writes:
[snip]
> > + /* If this node is now completely full, remove it from the free list. */
> > + if (node->bitmap == 0ull) {
> > + (void) util_dynarray_pop(vma_list, struct vma_bucket_node);
> > + }
> > +
> > + return node->start_address + bit * bucket->size;
>
> This looks like a use-after-free. It's not really unsafe because
> dynarray is just reducing .size, so it could only go bad if somebody
> else sneaks in there and _trims or _grows. So it's safe, but it hits the
> danger pattern matcher in my head.Whoops. Indeed, that should be safe, but it's also stupid. Fixed (by making a uint64_t addr = ... before popping and then using that). > Along similar lines, what is the mechanism that prevents multiple > threads from entering into brw_bo_alloc with the same bufmgr > simultaneously? The util/vma functions explode under simultaneous entry, > so I had to guard them with a mutex in anv. You don't have something > like that so I'm assuming something like the base mesa state tracking > code is keeping that from happening for you. The bufmgr->lock mutex is held on all paths calling vma_alloc or vma_free...except for brw_bufmgr_destroy, but there's no concurrency going on at final tear down. [snip] > > + /* Canonicalize the address. > > + * > > + * The Broadwell PRM Vol. 2a, MI_LOAD_REGISTER_MEM::MemoryAddress says: > > + * > > + * "This field specifies the address of the memory location where the > > + * register value specified in the DWord above will read from. The > > + * address specifies the DWord location of the data. Range = > > + * GraphicsVirtualAddress[63:2] for a DWord register GraphicsAddress > > + * [63:48] are ignored by the HW and assumed to be in correct > > + * canonical form [63:48] == [47]." > > + */ > > + const int shift = 63 - 47; > > + addr = (((int64_t) addr) << shift) >> shift; > > I updated this in anv to be: > > (int64_t)(addr << shift) >> shift; > > If addr happened to have any of the top 16 bits set then you will get > Undefined Behavior(tm) in C. Of course, that won't happen right here > because __vma_alloc always returns non-canonical addresses, but better > to have the fixed copy of the function sitting around. > > With that update, this is > > Reviewed-by: Scott D Phillips <scott.d.phill...@intel.com> Good call. If you move your copy into a common header, I'll just use that. For now, I've updated it to match your code.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
