linux-kernel - Re: [PATCH] virtio-balloon spec: provide a version of the "silent deflate" feature that works

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 7 Sep 2012 15:17:12 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	Rusty Russell <rusty@...tcorp.com.au>, fes@...gle.com,
	aarcange@...hat.com, riel@...hat.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, mikew@...gle.com, yinghan@...gle.com,
	virtualization@...ts.linux-foundation.org, yvugenfi@...hat.com,
	vrozenfe@...hat.com
Subject: Re: [PATCH] virtio-balloon spec: provide a version of the "silent
 deflate" feature that works

On Fri, Sep 07, 2012 at 01:20:57PM +0200, Paolo Bonzini wrote:
> Il 07/09/2012 12:53, Michael S. Tsirkin ha scritto:
> > Let us start with what is broken currently. Looking at
> > it very closely, I think the answer is nothing.
> > Even migration in qemu is not broken as you claimed initially.
> 
> Correct, migration would be broken as soon as QEMU starts using
> MUST_TELL_HOST.  I'm trying to think ahead, since we have many ideas
> floating around on the implementation of ballooning.
> 
> If you implement the mlock/munlock trick, you must start using
> MUST_TELL_HOST in QEMU to advertise it to guests, and migration breaks.

Migration does not break.

Since I wrote this code in qemu let me explain what is going on.

qemu requires that local and remote side are started with
same feature bits.
To support cross version migration, code in hw/pc_piix.c
disables features if you require migration from/to old qemu.

At some point I added a sanity check:
if we get guest features we know that any bit
set there must be set in host features.
Yes, this catches some user mistakes.

This was never intended as a compatibility guarantee.
User is still required to start qemu such
that host features match exactly, anything else
can lead to failures some of them hard to debug.

Here is a simple example:

1. guest reads host features
2. guest is migrated - check passes since no features are acked
3. guest acks features -> failure

This applies to any feature. Nothing special with this one.

Yes, we can if we want to make this more robust
against user errors, e.g. by migrating host feature
bits. Patches welcome. If we do it will help all
features, not just this one.

> > Next, consider the interface proposed here. You defacto declare
> > all existing drivers buggy.
> 
> No, only Windows (and it is buggy, it calls tell_host last).

It is not buggy. It does not ack MUST_TELL_HOST. So it is free to tell
host at any point, it behaves exactly
to spec. You can not retroactively declare drivers buggy like that.

>  Linux and
> BSD drivers do negotiate MUST_TELL_HOST, and are not buggy.
> 
> > This is a wrong thing to do.
> > You also use two feature bits for a single simple thing,
> > this is inelegant.
> 
> True, but the choice is:
> 
> 1) add a once-only hack to QEMU that fixes migration of
> VIRTIO_BALLOON_F_MUST_TELL_HOST;
> 
> 2) always advertise VIRTIO_BALLOON_F_MUST_TELL_HOST.  If you do this,
> guests cannot use anymore silent deflate, which is a regression.
> 
> 3) use two bits.  One tells the device that the driver supports chatty
> deflate; one tells the driver that the device supports silent deflate.

The right thing to do is either
4. realize we can not address all user errors, so no real issue
5. address this class of user errors by migrating host features

> So in the end you do use two feature bits for two different things.
> Plus, both feature bits are "positive" and I'm happy.

I am not happy.
We lose compatibility with all existing drivers
so it will take years until the feature is actually
useful.

> > Last, let us consider how existing feature can be used
> > in the hypervisor. If driver did not ack
> > MUST_TELL_HOST, it is *not* buggy but it means we can not
> > do munlock. This applies to current windows drivers.
> > If driver *did* ack MUST_TELL_HOST, we can munlock
> > and mlock back on leak.
> > Seems useful, driver support is already there,
> > so removing the MUST_TELL_HOST bit seems like a bad idea.
> 
> Indeed, repurposing MUST_TELL_HOST to WILL_TELL_HOST is better than
> killing it.
> 
> Paolo

Is this just a matter of naming? Same functionality:
driver that acks this bit will tell host first,
driver that does not will not?

If yes that is fine.

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