Linux Virtualization - Apr 2009 - [Lguest] [PATCH 4/5] lguest: use KVM hypercalls

fre, 27 03 2009 kl. 10:22 +1030, skrev Rusty Russell:
> From: Matias Zabaljauregui <zabaljauregui at gmail.com>
> 
> Impact: cleanup
> 
> This patch allow us to use KVM hypercalls
Something has broken in relation to this change. I'm not sure it is this
change itself or one following, but I get the following error when using
lguest:

lguest: unhandled trap 6 at 0x418726 (0x0)
> +static bool is_hypercall(struct lg_cpu *cpu)
> +{
> +	u8 insn[3];
> +
> +	/* This must be the Guest kernel trying to do something.
> +	 * The bottom two bits of the CS segment register are the privilege
> +	 * level. */
> +	if ((cpu->regs->cs & 3) != GUEST_PL)
> +		return false;
> +
> +	/* Is it a vmcall? */
> +	__lgread(cpu, insn, guest_pa(cpu, cpu->regs->eip), sizeof(insn));
I've put a printk for insn here that shows up twice. The first time insn
holds the values below, and the second time it holds the values that are
patched in by rewrite_hypercall.
> +	return insn[0] == 0x0f && insn[1] == 0x01 && insn[2] ==
0xc1;
> +}
I'll investigate further tomorrow, unless someone is already on top of
this. My kernel is 2.6.29-07100-g833bb30 btw.


Simon Holm Th?gersen

On Thu, 2009-04-02 at 23:55 +0200, Simon Holm Th?gersen
wrote:
> fre, 27 03 2009 kl. 10:22 +1030, skrev Rusty Russell:
> > From: Matias Zabaljauregui <zabaljauregui at gmail.com>
> > 
> > Impact: cleanup
> > 
> > This patch allow us to use KVM hypercalls
> 
> Something has broken in relation to this change. I'm not sure it is
this
> change itself or one following, but I get the following error when using
> lguest:
> 
> lguest: unhandled trap 6 at 0x418726 (0x0)

Hi, could you please attach your config? 
are you using the same kernel for guest and host?

I just tested it with 2.6.29-07100-g833bb30  and works OK for me.

thanks
Matias

On Friday 03 April 2009 08:25:24 Simon Holm Th?gersen
wrote:
> fre, 27 03 2009 kl. 10:22 +1030, skrev Rusty Russell:
> > From: Matias Zabaljauregui <zabaljauregui at gmail.com>
> > 
> > Impact: cleanup
> > 
> > This patch allow us to use KVM hypercalls
> 
> Something has broken in relation to this change. I'm not sure it is
this
> change itself or one following, but I get the following error when using
> lguest:
> 
> lguest: unhandled trap 6 at 0x418726 (0x0)
Any chance you didn't rebuild the Launcher?

Rusty.

Simon, Patrick,

On Wed, 2009-04-08 at 10:21 +0930, Rusty Russell wrote:
> On Tuesday 07 April 2009 04:19:58 Matias Zabaljauregui wrote:
> > But then, when it tries to re-execute the faulting instruction, which
is
> > now patched with "cd 1f 90"  (int 0x1f, nop), it raises an
invalid code
> > fault again, instead of doing the trap. 
> 
> COW on the page?  Perhaps try flushing all the shadow pagetables after a
> rewrite?
> 
> Rusty.
Rusty suggested that perhaps we need to flush all shadow after rewriting. 
Could you please try this patch?

BTW, shouldn't this also affect my test boxes if this were the case?


Thanks,
Matias




diff --git a/drivers/lguest/x86/core.c b/drivers/lguest/x86/core.c
index a6b7176..b4747f7 100644
--- a/drivers/lguest/x86/core.c
+++ b/drivers/lguest/x86/core.c
@@ -324,6 +324,7 @@ static void rewrite_hypercall(struct lg_cpu *cpu)
 	u8 insn[3] = {0xcd, 0x1f, 0x90};
 
 	__lgwrite(cpu, guest_pa(cpu, cpu->regs->eip), insn, sizeof(insn));
+	guest_pagetable_clear_all(cpu);
 }
 
 static bool is_hypercall(struct lg_cpu *cpu)

On Wed, Apr 15, 2009 at 04:36:10PM +0800, Herbert Xu
wrote:
> 
> Let me whip up a patch.
tun: Fix sk_sleep races when attaching/detaching

As the sk_sleep wait queue actually lives in tfile, which may be
detached from the tun device, bad things will happen when we use
sk_sleep after detaching.

Since the tun device is the persistent data structure here (when
requested by the user), it makes much more sense to have the wait
queue live there.  There is no reason to have it in tfile at all
since the only time we can wait is if we have a tun attached.
In fact we already have a wait queue in tun_struct, so we might
as well use it.

Reported-by: Christian Borntraeger <borntraeger at de.ibm.com>
Reported-by: Eric W. Biederman <ebiederm at xmission.com>
Reported-by: Patrick McHardy <kaber at trash.net>
Signed-off-by: Herbert Xu <herbert at gondor.apana.org.au>

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a1b0697..412b578 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -93,7 +93,6 @@ struct tun_file {
 	atomic_t count;
 	struct tun_struct *tun;
 	struct net *net;
-	wait_queue_head_t	read_wait;
 };
 
 struct tun_sock;
@@ -333,7 +332,7 @@ static void tun_net_uninit(struct net_device *dev)
 	/* Inform the methods they need to stop using the dev.
 	 */
 	if (tfile) {
-		wake_up_all(&tfile->read_wait);
+		wake_up_all(&tun->socket.wait);
 		if (atomic_dec_and_test(&tfile->count))
 			__tun_detach(tun);
 	}
@@ -393,7 +392,7 @@ static int tun_net_xmit(struct sk_buff *skb, struct
net_device *dev)
 	/* Notify and wake up reader process */
 	if (tun->flags & TUN_FASYNC)
 		kill_fasync(&tun->fasync, SIGIO, POLL_IN);
-	wake_up_interruptible(&tun->tfile->read_wait);
+	wake_up_interruptible(&tun->socket.wait);
 	return 0;
 
 drop:
@@ -490,7 +489,7 @@ static unsigned int tun_chr_poll(struct file *file,
poll_table * wait)
 
 	DBG(KERN_INFO "%s: tun_chr_poll\n", tun->dev->name);
 
-	poll_wait(file, &tfile->read_wait, wait);
+	poll_wait(file, &tun->socket.wait, wait);
 
 	if (!skb_queue_empty(&tun->readq))
 		mask |= POLLIN | POLLRDNORM;
@@ -762,7 +761,7 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const
struct iovec *iv,
 		goto out;
 	}
 
-	add_wait_queue(&tfile->read_wait, &wait);
+	add_wait_queue(&tun->socket.wait, &wait);
 	while (len) {
 		current->state = TASK_INTERRUPTIBLE;
 
@@ -793,7 +792,7 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const
struct iovec *iv,
 	}
 
 	current->state = TASK_RUNNING;
-	remove_wait_queue(&tfile->read_wait, &wait);
+	remove_wait_queue(&tun->socket.wait, &wait);
 
 out:
 	tun_put(tun);
@@ -861,7 +860,6 @@ static int tun_set_iff(struct net *net, struct file *file,
struct ifreq *ifr)
 	struct sock *sk;
 	struct tun_struct *tun;
 	struct net_device *dev;
-	struct tun_file *tfile = file->private_data;
 	int err;
 
 	dev = __dev_get_by_name(net, ifr->ifr_name);
@@ -921,11 +919,11 @@ static int tun_set_iff(struct net *net, struct file *file,
struct ifreq *ifr)
 
 		/* This ref count is for tun->sk. */
 		dev_hold(dev);
+		init_waitqueue_head(&tun->socket.wait);
 		sock_init_data(&tun->socket, sk);
 		sk->sk_write_space = tun_sock_write_space;
 		sk->sk_destruct = tun_sock_destruct;
 		sk->sk_sndbuf = INT_MAX;
-		sk->sk_sleep = &tfile->read_wait;
 
 		tun->sk = sk;
 		container_of(sk, struct tun_sock, sk)->tun = tun;
@@ -1265,7 +1263,6 @@ static int tun_chr_open(struct inode *inode, struct file *
file)
 	atomic_set(&tfile->count, 0);
 	tfile->tun = NULL;
 	tfile->net = get_net(current->nsproxy->net_ns);
-	init_waitqueue_head(&tfile->read_wait);
 	file->private_data = tfile;
 	return 0;
 }

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert at gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Am Wednesday 15 April 2009 10:47:17 schrieb Herbert Xu:
> tun: Fix sk_sleep races when attaching/detaching
>
> As the sk_sleep wait queue actually lives in tfile, which may be
> detached from the tun device, bad things will happen when we use
> sk_sleep after detaching.
>
> Since the tun device is the persistent data structure here (when
> requested by the user), it makes much more sense to have the wait
> queue live there.  There is no reason to have it in tfile at all
> since the only time we can wait is if we have a tun attached.
> In fact we already have a wait queue in tun_struct, so we might
> as well use it.
>
> Reported-by: Christian Borntraeger <borntraeger at de.ibm.com>
> Reported-by: Eric W. Biederman <ebiederm at xmission.com>
> Reported-by: Patrick McHardy <kaber at trash.net>
> Signed-off-by: Herbert Xu <herbert at gondor.apana.org.au>
Tested-by: Christian Borntraeger <borntraeger at de.ibm.com>

Herbert Xu wrote:
> On Wed, Apr 15, 2009 at 04:36:10PM +0800, Herbert Xu wrote:
>> Let me whip up a patch.
> 
> tun: Fix sk_sleep races when attaching/detaching
> 
> As the sk_sleep wait queue actually lives in tfile, which may be
> detached from the tun device, bad things will happen when we use
> sk_sleep after detaching.
> 
> Since the tun device is the persistent data structure here (when
> requested by the user), it makes much more sense to have the wait
> queue live there.  There is no reason to have it in tfile at all
> since the only time we can wait is if we have a tun attached.
> In fact we already have a wait queue in tun_struct, so we might
> as well use it.
Tested and works fine, thanks Herbert.

From: Herbert Xu <herbert at gondor.apana.org.au>
Date: Mon, 20 Apr 2009 17:35:49 +0800
> On Mon, Apr 20, 2009 at 02:26:35AM -0700, David Miller wrote:
>> 
>> Do you think these two patches are ready to go into net-2.6
>> now?
> 
> I think so.
Great, applied, thanks Herbert.