Re: [PATCH -mm] swsusp: support creating bigger images

[Hugh Dickins]

On Tue, 9 May 2006, Rafael J. Wysocki wrote:
> On Tuesday 09 May 2006 09:33, Andrew Morton wrote:
> > "Rafael J. Wysocki" <rjw@xxxxxxx> wrote:
> > 
> > I have a host of minor problems with this patch.
> > 
> > > --- linux-2.6.17-rc3-mm1.orig/mm/rmap.c
> > > +++ linux-2.6.17-rc3-mm1/mm/rmap.c
> > >  
> > > +#ifdef CONFIG_SOFTWARE_SUSPEND
> > > +static int page_mapped_by_task(struct page *page, struct task_struct *task)
> > > +{
> > > +	struct vm_area_struct *vma;
> > > +	int ret = 0;
> > > +
> > > +	spin_lock(&task->mm->page_table_lock);
> > > +
> > > +	for (vma = task->mm->mmap; vma; vma = vma->vm_next)
> > > +		if (page_address_in_vma(page, vma) != -EFAULT) {
> > > +			ret = 1;
> > > +			break;
> > > +		}
> > > +
> > > +	spin_unlock(&task->mm->page_table_lock);
> > > +
> > > +	return ret;
> > > +}
> > 
> > task_struct.mm can sometimes be NULL.  This function assumes that it will
> > never be NULL.  That makes it a somewhat risky interface.  Are we sure it
> > can never be NULL?
> 
> Well, now it's only called for task == current, but I can add a check.

Better fold it into the (renamed and recommented) page_to_copy,
applying only to current.

The "use" of page_table_lock there is totally bogus.  Normally you
need down_read(&current->mm->mmap_sem) to walk that vma chain; but
I'm guessing you have everything sufficiently frozen here that you
don't need that.

But if it is sufficiently frozen, I'm puzzled as to why pages mapped
into the current process are (potentially) unsafe, while those mapped
into others are safe.  If the current process can get back to messing
with its mapped pages, what if it maps a page you earlier judged safe?

Hugh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/