Hi Matt, On Fri, Oct 14, 2022 at 10:44:11AM +0100, Matthew Auld wrote: > On 14/10/2022 09:56, Andi Shyti wrote: > > On Fri, Oct 14, 2022 at 09:39:52AM +0100, Matthew Auld wrote: > > > On 13/10/2022 18:56, Jonathan Cavitt wrote: > > > > i915_ttm_to_gem can return a NULL pointer, which is > > > > dereferenced in i915_ttm_access_memory without first > > > > checking if it is NULL. Inspecting > > > > i915_ttm_io_mem_reserve, it appears the correct > > > > behavior in this case is to return -EINVAL. > > > > > > The GEM object has already been dereferenced before this point, if you look > > > at the caller (vm_access_ttm). The NULL obj thing is to identify "ttm ghost > > > objects", and I don't think a normal userpace object can suddenly become one > > > (access_memory comes from ptrace). AFAIK ghost objects are just for > > > temporarily hanging on to some memory/state, while the dma-resv is busy. In > > > the places where ttm is the one giving us the object, then it might be > > > possible to see these types of objects, since ttm could in theory pass one > > > in (like during eviction). > > > > True that, but because from a code persepctive we can still receive > > NULL, I think the check is correct, perhaps we could: > > > > if (unlikely(!obj)) > > return -EINVAL; > > Hmm, so that will dereference some pointer, and then later check if it is > NULL here? Or do you mean to move this into vm_access()? If we are given a > "ghost object" for ptrace this would likely mean we have a very nasty bug > somewhere (unless I'm misunderstanding something), and so returning a normal > user error here doesn't seem right to me (maybe this just hides the issue)? > Letting it crash seems fine to me tbh. It also makes the code harder to > understand IMO, because looking at this it now suggests that it is somehow > possible to have a "ghost object" here. Also there are a fair few places > calling i915_ttm_to_gem() which already don't check for NULL, since it > should be impossible, like it should be here. By just analyzing the code, getting NULL is not impossible. In that case even a GEM_BUG_ON would have worked. But the NULL pointer, as it is, needs to be checked. Anyway, I see that an agreement has been reached with Nirmoy, so that it doesn't matter anymore :) Andi