Michel Dänzer wrote:
> On Wed, 2009-08-05 at 11:18 +0200, Thomas Hellström wrote:
>
>> Michel Dänzer wrote:
>>
>>> On Wed, 2009-08-05 at 11:01 +0200, Thomas Hellström wrote:
>>>
>>>
>>>> Michel Dänzer wrote:
>>>>
>>>>
>>>>> On Wed, 2009-08-05 at 10:50 +0200, Thomas Hellström wrote:
>>>>>
>>>>>
>>>>>
>>>>>> Michel Dänzer wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> From: Michel Dänzer <daen...@vmware.com>
>>>>>>>
>>>>>>> Make sure bo->vm_node is valid, to prevent crashes in the fault
>>>>>>> handler, and
>>>>>>> adjust vma->vm_pgoff according to it, so userspace attempts to access
>>>>>>> /dev/fb*
>>>>>>> mappings don't always result in SIGBUS.
>>>>>>>
>>>>>>> Signed-off-by: Michel Dänzer <daen...@vmware.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/ttm/ttm_bo_vm.c | 6 ++++++
>>>>>>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>>>>> b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>>>>> index 27b146c..57d0c94 100644
>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
>>>>>>> @@ -282,7 +282,13 @@ int ttm_fbdev_mmap(struct vm_area_struct *vma,
>>>>>>> struct ttm_buffer_object *bo)
>>>>>>> if (vma->vm_pgoff != 0)
>>>>>>> return -EACCES;
>>>>>>>
>>>>>>> + if (!bo->vm_node) {
>>>>>>> + printk(KERN_ERR TTM_PFX "bo->vm_node == NULL\n");
>>>>>>> + return -EACCES;
>>>>>>> + }
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Michel,
>>>>>> Did you actually hit a bo with an invalid vm_node?
>>>>>>
>>>>>>
>>>>>>
>>>>> See patch 3 - the radeon driver used to create the fbcon BO as
>>>>> kernel-only, so when I tried passing that to ttm_fbdev_mmap()
>>>>> all /dev/fb* userspace mappings mysteriously caught SIGBUS on access.
>>>>>
>>>>>
>>> Actually, it was even worse - the TTM fault handler crashed because it
>>> unconditionally dereferences bo->vm_node.
>>>
>>>
>>>
>>>>> It took me a while to track that down, this is basically a debugging aid
>>>>> for bugs like that.
>>>>>
>>>>>
>>>>>
>>>> Ah, OK. But since that is really a kernel bug, we should trap it with
>>>> BUG_ON().
>>>>
>>>>
>>> Good point, will do.
>>>
>>>
>>>
>>>
>> Aargh. Wait. I remember now.
>>
>> The fbcon bo is exported through the fbdev address space at offset 0.
>> The vm_node is for the drm device address space only. So it is perfectly
>> legal and actually correct for it not to have a vm_node, unless it's
>> going to be accessible from the drm device. Does it need to for KMS?
>>
>
> I don't think so.
>
>
>> I'm a bit unsure whether it's OK to export a bo through two different
>> address spaces. In particular, unmap_mapping_range() on the drm device
>> will not kill the fbdev user-space mappings.
>>
>
> Hmm, so that would mean that if an fbdev mapping is created while the BO
> is in VRAM, it would still access VRAM after the BO has been evicted? Is
> there a solution for this?
>
>
Yes, You need to call unmap_mapping_range() on the fbdev address space.
See how that is done in ttm_bo_unmap_virtual() for the drm address
space. Actually, I think you need to set up a bo_driver hook in
ttm_bo_unmap_virtual() to do this every time the bo is moved or swapped out.
This is how psb got hold on the fbdev address space. The locking of bo->
mutex is not valid anymore. I just used the bo_mutex to protect the
access of psbfb->addr_space from simultaneous readers and writers.
static int psbfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
struct psbfb_par *par = info->par;
struct psb_framebuffer *psbfb = par->psbfb;
struct ttm_buffer_object *bo = psbfb->bo;
unsigned long size = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
unsigned long offset = vma->vm_pgoff;
if (vma->vm_pgoff != 0)
return -EINVAL;
if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
return -EINVAL;
if (offset + size > bo->num_pages)
return -EINVAL;
mutex_lock(&bo->mutex);
if (!psbfb->addr_space)
psbfb->addr_space = vma->vm_file->f_mapping;
mutex_unlock(&bo->mutex);
return ttm_fbdev_mmap(vma, bo);
}
/Thomas
>> The bug is in the ttm fbdev fault handler which is the same as the drm
>> device fault handler that uses the vm_node->offset when it should
>> actually use 0. We should probably add an is_fbdev argument to the fault
>> handler and wrap it for its two uses.
>>
>
> Or maybe it could just check if vm_node is non-NULL and use 0 otherwise.
> I'll defer to you which is better.
>
>
>
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
