[PATCH 1/1 2.6.25] fbdev: bugfix for multiprocess defio

[PATCH 1/1 2.6.25] fbdev: bugfix for multiprocess defio

[Jaya Kumar]

Hi Tony, Geert, Andrew, fbdev,

This patch is a bugfix for how defio handles multiple processes manipulating
the same framebuffer. Thanks to Bernard Blackham for identifying this bug.
It occurs when two applications mmap the same framebuffer and concurrently
write to the same page. Normally, this doesn't occur since only a single
process mmaps the framebuffer. The symptom of the bug is that the mapping
applications will hang. The cause is that defio incorrectly tries to add the
same page twice to the pagelist. The solution I have is to walk the pagelist
and check for a duplicate before adding. Since I needed to walk the
pagelist, I now also keep the pagelist in sorted order.

Thanks,
jaya

Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com>

diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
index 24843fd..59df132 100644
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -74,6 +74,7 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
 {
 	struct fb_info *info = vma->vm_private_data;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
+	struct page *cur;
 
 	/* this is a callback we get when userspace first tries to
 	write to the page. we schedule a workqueue. that workqueue
@@ -83,7 +84,24 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
 
 	/* protect against the workqueue changing the page list */
 	mutex_lock(&fbdefio->lock);
-	list_add(&page->lru, &fbdefio->pagelist);
+
+	/* we loop through the pagelist before adding in order
+	to keep the pagelist sorted */
+	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+		/* this check is to catch the case where a new
+		process could start writing to the same page
+		through a new pte. this new access can cause the
+		mkwrite even when the original ps's pte is marked
+		writable */
+		if (unlikely(cur == page))
+			goto page_already_added;
+		else if (cur->index > page->index)
+			break;
+	}
+
+	list_add_tail(&page->lru, &cur->lru);
+
+page_already_added:
 	mutex_unlock(&fbdefio->lock);
 
 	/* come back after delay to process the deferred IO */

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH 1/1 2.6.25] fbdev: bugfix for multiprocess defio

[Andrew Morton]

On Sun, 27 Apr 2008 22:01:40 -0400
Jaya Kumar <jayakumar.lkml@gmail.com> wrote:

> Hi Tony, Geert, Andrew, fbdev,
> 
> This patch is a bugfix for how defio handles multiple processes manipulating
> the same framebuffer. Thanks to Bernard Blackham for identifying this bug.
> It occurs when two applications mmap the same framebuffer and concurrently
> write to the same page. Normally, this doesn't occur since only a single
> process mmaps the framebuffer. The symptom of the bug is that the mapping
> applications will hang. The cause is that defio incorrectly tries to add the
> same page twice to the pagelist. The solution I have is to walk the pagelist
> and check for a duplicate before adding. Since I needed to walk the
> pagelist, I now also keep the pagelist in sorted order.
> 
> Thanks,
> jaya
> 
> Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com>
> 
> diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
> index 24843fd..59df132 100644
> --- a/drivers/video/fb_defio.c
> +++ b/drivers/video/fb_defio.c
> @@ -74,6 +74,7 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
>  {
>  	struct fb_info *info = vma->vm_private_data;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
> +	struct page *cur;
>  
>  	/* this is a callback we get when userspace first tries to
>  	write to the page. we schedule a workqueue. that workqueue
> @@ -83,7 +84,24 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
>  
>  	/* protect against the workqueue changing the page list */
>  	mutex_lock(&fbdefio->lock);
> -	list_add(&page->lru, &fbdefio->pagelist);
> +
> +	/* we loop through the pagelist before adding in order
> +	to keep the pagelist sorted */
> +	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> +		/* this check is to catch the case where a new
> +		process could start writing to the same page
> +		through a new pte. this new access can cause the
> +		mkwrite even when the original ps's pte is marked
> +		writable */
> +		if (unlikely(cur == page))
> +			goto page_already_added;
> +		else if (cur->index > page->index)
> +			break;
> +	}
> +
> +	list_add_tail(&page->lru, &cur->lru);
> +
> +page_already_added:
>  	mutex_unlock(&fbdefio->lock);
>  

Did you consider using !list_empty(&page->lru) to avoid the linear search?

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH 1/1 2.6.25] fbdev: bugfix for multiprocess defio

[Jaya Kumar]

On Wed, Apr 30, 2008 at 1:37 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Sun, 27 Apr 2008 22:01:40 -0400
>  Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
>
>  > +     /* we loop through the pagelist before adding in order
>  > +     to keep the pagelist sorted */
>  > +     list_for_each_entry(cur, &fbdefio->pagelist, lru) {
>
>  Did you consider using !list_empty(&page->lru) to avoid the linear search?
>

Ah, no, I didn't think of that. I agree that checking list_empty would
be far better. I'll redo the patch using that.

Thanks,
jaya

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: [PATCH 1/1 2.6.25] fbdev: bugfix for multiprocess defio

[Jaya Kumar]

On Mon, May 5, 2008 at 3:23 AM, Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
> On Wed, Apr 30, 2008 at 1:37 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>>
>> On Sun, 27 Apr 2008 22:01:40 -0400
>>  Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
>>
>>  > +     /* we loop through the pagelist before adding in order
>>  > +     to keep the pagelist sorted */
>>  > +     list_for_each_entry(cur, &fbdefio->pagelist, lru) {
>>
>>  Did you consider using !list_empty(&page->lru) to avoid the linear search?
>>
>
> Ah, no, I didn't think of that. I agree that checking list_empty would
> be far better. I'll redo the patch using that.
>
> Thanks,
> jaya
>

Hi Andrew,

I encountered some complexity when trying to use page->lru because its
not empty on its first mkwrite. I think there are probably deeper
issues that repurposing page->lru for use in defio causes. I'm working
on it.

In the meantime, it turns out that suspend/resume of xen pvfb is
affected by the same bug that raised this patch. Markus suggested,
http://marc.info/?l=linux-kernel&m=121368814027583&w=2 , that perhaps
this existing patch could be merged in the interim while a better
solution is worked on.

Would you be okay with adding this patch back into the queue? If so,
should I repost it here?

Thanks,
jaya

-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08

Re: [PATCH 1/1 2.6.25] fbdev: bugfix for multiprocess defio

[Andrew Morton]

On Tue, 8 Jul 2008 08:50:30 -0400 "Jaya Kumar" <jayakumar.lkml@gmail.com> wrote:

> On Mon, May 5, 2008 at 3:23 AM, Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
> > On Wed, Apr 30, 2008 at 1:37 PM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> >>
> >> On Sun, 27 Apr 2008 22:01:40 -0400
> >>  Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
> >>
> >>  > +     /* we loop through the pagelist before adding in order
> >>  > +     to keep the pagelist sorted */
> >>  > +     list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> >>
> >>  Did you consider using !list_empty(&page->lru) to avoid the linear search?
> >>
> >
> > Ah, no, I didn't think of that. I agree that checking list_empty would
> > be far better. I'll redo the patch using that.
> >
> > Thanks,
> > jaya
> >
> 
> Hi Andrew,
> 
> I encountered some complexity when trying to use page->lru because its
> not empty on its first mkwrite. I think there are probably deeper
> issues that repurposing page->lru for use in defio causes. I'm working
> on it.
> 
> In the meantime, it turns out that suspend/resume of xen pvfb is
> affected by the same bug that raised this patch. Markus suggested,
> http://marc.info/?l=linux-kernel&m=121368814027583&w=2 , that perhaps
> this existing patch could be merged in the interim while a better
> solution is worked on.
> 
> Would you be okay with adding this patch back into the queue? If so,
> should I repost it here?

It never hurts to resend everything.  Especially when "everything" is
several months old!

-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08

[PATCH 1/1 2.6.25] fbdev: bugfix for multiprocess defio

[Jaya Kumar]

This patch is a bugfix for how defio handles multiple processes manipulating
the same framebuffer.

Thanks to Bernard Blackham for identifying this bug.

It occurs when two applications mmap the same framebuffer and concurrently
write to the same page. Normally, this doesn't occur since only a single
process mmaps the framebuffer. The symptom of the bug is that the mapping
applications will hang. The cause is that defio incorrectly tries to add the
same page twice to the pagelist. The solution I have is to walk the pagelist
and check for a duplicate before adding. Since I needed to walk the
pagelist, I now also keep the pagelist in sorted order.

Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com>

diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
index 24843fd..59df132 100644
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -74,6 +74,7 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
 {
 	struct fb_info *info = vma->vm_private_data;
 	struct fb_deferred_io *fbdefio = info->fbdefio;
+	struct page *cur;
 
 	/* this is a callback we get when userspace first tries to
 	write to the page. we schedule a workqueue. that workqueue
@@ -83,7 +84,24 @@ static int fb_deferred_io_mkwrite(struct vm_area_struct *vma,
 
 	/* protect against the workqueue changing the page list */
 	mutex_lock(&fbdefio->lock);
-	list_add(&page->lru, &fbdefio->pagelist);
+
+	/* we loop through the pagelist before adding in order
+	to keep the pagelist sorted */
+	list_for_each_entry(cur, &fbdefio->pagelist, lru) {
+		/* this check is to catch the case where a new
+		process could start writing to the same page
+		through a new pte. this new access can cause the
+		mkwrite even when the original ps's pte is marked
+		writable */
+		if (unlikely(cur == page))
+			goto page_already_added;
+		else if (cur->index > page->index)
+			break;
+	}
+
+	list_add_tail(&page->lru, &cur->lru);
+
+page_already_added:
 	mutex_unlock(&fbdefio->lock);
 
 	/* come back after delay to process the deferred IO */

-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08