Re: [Xen-devel] [Patch] fix xenfb_update_screen bogus rect

[Markus Armbruster <armbru@xxxxxxxxxx>]

Akio Takebe <takebe_akio@xxxxxxxxxxxxxx> writes:

> Hi, Markus
>
> Markus Armbruster wrote:
>> Akio Takebe <takebe_akio@xxxxxxxxxxxxxx> writes:
[...]
>>> diff -r 83b71f4b5cb2 drivers/xen/fbfront/xenfb.c
>>> --- a/drivers/xen/fbfront/xenfb.c   Tue Jan 20 13:28:35 2009 +0000
>>> +++ b/drivers/xen/fbfront/xenfb.c   Thu Jan 29 01:24:06 2009 +0900
>>> @@ -213,17 +213,23 @@
>>
>> Please use -p with diff.
>>
>>>     if (xenfb_queue_full(info))
>>>             return;
>>>  -  mutex_lock(&info->mm_lock);
>>> -
>>>     spin_lock_irqsave(&info->dirty_lock, flags);
>>> -   y1 = info->y1;
>>> -   y2 = info->y2;
>>> -   x1 = info->x1;
>>> -   x2 = info->x2;
>>> -   info->x1 = info->y1 = INT_MAX;
>>> -   info->x2 = info->y2 = 0;
>>> +   if (info->dirty){
>>> +           info->dirty = 0;
>>> +           y1 = info->y1;
>>> +           y2 = info->y2;
>>> +           x1 = info->x1;
>>> +           x2 = info->x2;
>>> +           info->x1 = info->y1 = INT_MAX;
>>> +           info->x2 = info->y2 = 0;
>>> +   } else {
>>> +           spin_unlock_irqrestore(&info->dirty_lock, flags);
>>> +           return;
>>> +   }
>>>     spin_unlock_irqrestore(&info->dirty_lock, flags);
>>>  +  mutex_lock(&info->mm_lock);
>>> +   
>>>     list_for_each_entry(map, &info->mappings, link) {
>>>             if (!map->faults)
>>>                     continue;
>>
>> Careful, locking is rather delicate here.  Please read the big comment
>> "There are three locks:", then explain why moving the locking of
>> mm_lock is safe.
>>
> Thank you for your review.

My pleasure.  I have to thank for your fix!

> First, I thought mm_lock just protected the mappings,
> and the dirty rectangle was protected by the dirty_lock.
> So I moved the mm_lock.
> Also when I tested the patch, I didn't get any problem.

Subtle locking bugs are notoriously hard to demonstrate by testing.

> Do you mean the narrow point of between unloking dirty_lock and locking 
> mm_lock
> in xenfb_update_screen() may be not safe?
> Do you find any critical cases?
>
> Best Regards,
>
> Akio Takebe

Your proposed change might be fine, it might be racy, I don't know.
What I do know is that it invalidates the comment I mentioned.  That's
a nasty trap for the unwary, and clearly a bug as far as I'm
concerned.

I strongly recommend to either revert the locking change, or fix the
comment to match the new code.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel