Bug#674411: linux-image-2.6.32-5-xen-686: Kernel crashes if AIO is used on pages belonging to guests

[Ian Campbell <ijc@hellion.org.uk>]

On Thu, 2012-05-31 at 11:37 +0100, Stefano Stabellini wrote:
> On Wed, 30 May 2012, Ian Campbell wrote:
> > On Thu, 2012-05-24 at 11:57 +0000, George Dunlap wrote:
> > > Package: linux-2.6
> > > Version: 2.6.32-45
> > > Severity: normal
> > > 
> > > The kernel version in this package does not handle AIO to pages 
> > > owned by guest domains ("foreign domains") properly.  Newer versions of qemu
> > > will attempt to use AIO for disk reads and writes; this causes domain 0 to crash
> > > if you're running.
> > 
> > Thanks George. Do you happen to have a log of the actual kernel crash?
> > 
> > I think Stefano (CCd) has been fixing this issue (or at least one which
> > sounds very similar) in the upstream kernels recently. Stefano is there
> > anything specific I can backport to a Jeremy xen.git 2.6.32 style pvops
> > kernel? Or is there another appropriate fix?
> > 
> > FYI Squeeze's kernel is based on e73f4955a821 from Jeremy's tree,
> > although with a bunch of fixes backported too.
> 
> We need to backport the m2p_override, this is a list of the relevant
> upstream commits:
> 
> 289b777eac19c811b474593b4d2fd14e46340c23
> b5eafe924bb054d7c56e6ebd18106352e8a3f916
> 448f2831934381e9d3c4d93e700ba7bbe14612dc
> 9b705f0e98c489b18ba22a6eab9d694b546c8552
> 87f1d40a706bdebdc8f959b9ac291d0d8fdfcc7e
> e1b478e4ec4477520767d1a920433626263a2a6b
> b254244d2682fe975630f176c25a4444cc4e088d
> cf8d91633ddef9e816ccbf3da833c79ce508988d
> 0f4b49eaf25e661fbe63a5370b7781166b34d616
> 0930bba674e248b921ea659b036ff02564e5a5f4
> dc4972a4e2f3fee1663bd0670dfc4cd798d5f9b2
> 
> 
> The backport is non-trivial, but I can help.

I think that might be a bit drastic for Squeeze at this point...

> As an alternative we could add a simple check to spot an attempt to use
> AIO on a granted page and return an error (still better than crashing):

It's a bit skanky but I am slightly leaning towards it.

> ---
> 
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index a33e443..f060770 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -573,6 +573,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma)
>  	vma->vm_flags |= VM_RESERVED;
>  	vma->vm_flags |= VM_DONTCOPY;
>  	vma->vm_flags |= VM_DONTEXPAND;
> +	vma->vm_flags |= VM_FOREIGN;
>  
>  	vma->vm_private_data = map;
>  	map->vma = vma;
> diff --git a/fs/aio.c b/fs/aio.c
> index 02a2c93..70b8854 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1655,6 +1655,8 @@ SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
>  	for (i=0; i<nr; i++) {
>  		struct iocb __user *user_iocb;
>  		struct iocb tmp;
> +		struct vm_area_struct *vma = NULL;
> +		struct iovec *v = NULL;
>  
>  		if (unlikely(__get_user(user_iocb, iocbpp + i))) {
>  			ret = -EFAULT;
> @@ -1666,6 +1668,19 @@ SYSCALL_DEFINE3(io_submit, aio_context_t, ctx_id, long, nr,
>  			break;
>  		}
>  
> +		down_read(&current->mm->mmap_sem);
> +		v = (struct iovec *) tmp.aio_buf;
> +		/* just checking the first iovec is enough for now */
> +		if (v != NULL)
> +			vma = find_vma(current->mm, (unsigned long) v->iov_base);
> +		if (vma != NULL && vma->vm_flags & (VM_FOREIGN|VM_DONTEXPAND)) {

Do you mean "flags & (FOREIGN|DONTEXPACT) == (FOREIGN|DONTEXPAND)" or is
either one being set a bad thing?

> +			WARN_ONCE(1, "missing AIO support on granted pages\n");
> +			ret = -EFAULT;
> +			up_read(&current->mm->mmap_sem);
> +			break;
> +		}
> +		up_read(&current->mm->mmap_sem);
> +
>  		ret = io_submit_one(ctx, user_iocb, &tmp);
>  		if (ret)
>  			break;
> 
> 
> 

-- 
Ian Campbell
Current Noise: Guns n' Roses - Sweet Child O' Mine

Set the cart before the horse.
		-- John Heywood