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

[Akio Takebe <takebe_akio@xxxxxxxxxxxxxx>]

Hi, Markus

Markus Armbruster wrote:
> 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.
I agree with you. I revert the mm_lock changing.
In my graphic test, I didn't get any problem with this patch.

Signed-off-by: Akio Takebe <takebe_akio@xxxxxxxxxxxxxx>

Best Regards,

Akio Takebe

diff -r f90f02049d7e drivers/xen/fbfront/xenfb.c
--- a/drivers/xen/fbfront/xenfb.c       Thu Jan 29 18:26:47 2009 +0900
+++ b/drivers/xen/fbfront/xenfb.c       Fri Jan 30 10:54:07 2009 +0900
@@ -210,6 +210,8 @@ static void xenfb_update_screen(struct x
        if (xenfb_queue_full(info))
                return;
 
+       mutex_lock(&info->mm_lock);
+       
        spin_lock_irqsave(&info->dirty_lock, flags);
        if (info->dirty){
                info->dirty = 0;
@@ -221,12 +223,11 @@ static void xenfb_update_screen(struct x
                info->x2 = info->y2 = 0;
        } else {
                spin_unlock_irqrestore(&info->dirty_lock, flags);
+               mutex_unlock(&info->mm_lock);
                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;

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