From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58406) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c461e-00037x-FJ for qemu-devel@nongnu.org; Tue, 08 Nov 2016 08:01:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c461c-0004rW-Qi for qemu-devel@nongnu.org; Tue, 08 Nov 2016 08:01:18 -0500 Received: from mail-lf0-x230.google.com ([2a00:1450:4010:c07::230]:35756) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c461c-0004pq-Bf for qemu-devel@nongnu.org; Tue, 08 Nov 2016 08:01:16 -0500 Received: by mail-lf0-x230.google.com with SMTP id b14so138517827lfg.2 for ; Tue, 08 Nov 2016 05:01:16 -0800 (PST) MIME-Version: 1.0 References: <20161022070041.23763-1-rafael.tinoco@canonical.com> <20161030192651.sh7gydgo6r4a42ml@redhat.com> <20161101002844-mutt-send-email-mst@kernel.org> In-Reply-To: From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Tue, 08 Nov 2016 13:01:03 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] vhost: secure vhost shared log files using argv paremeter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Rafael David Tinoco , "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org, 1626972@bugs.launchpad.net Hi On Tue, Nov 8, 2016 at 4:49 PM Rafael David Tinoco < rafael.tinoco@canonical.com> wrote: > Hello Michael, Andr=C3=A9, > > Could you do a quick review before a final submission ? > > http://paste.ubuntu.com/23446279/ > > - I split the commits into 1) bugfix, 2) new util with test, 3) vhostlog > > The unit test is testing passing fds between 2 processes and asserting > contents of mmap buffer coming from the "vhostlog" util (mmap-file). > > Your final comment on the "vhostlog" was: > > >> Argv examples: > >> > >> -netdev tap,id=3Dnet0,vhost=3Don > >> -netdev tap,id=3Dnet0,vhost=3Don,vhostlog=3D/tmp/guest.log > >> -netdev tap,id=3Dnet0,vhost=3Don,vhostlog=3D/tmp > > (Andr=C3=A9) > Could it be only a filename? This would simplify testing. > (Michael) > When vhostlog is not specified, can we just use memfd as we > did? > > Michael said: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg08197.html I think that the best approach is to allow passing in the fd, not the file path. If not passed, use memfd. I do agree :) I'm going to change this to: > > 1 - if vhostlog is not provided shared log can't be used. Use memfd. > > 2 - for shared logs, vhostlog has to be provided as a "file" ? > > Should i keep vhostlog being a directory also ? (i know we are unlinking > the > file so might not be needed BUT a static file might have a race condition > in > between different instances and providing a directory - that creates rand= om > files on it - might be better approach). > > Is there anything else ? > Do we really need to give a path? (pass fd with -add-fd/qmp add-fd) Thank you > > Rafael Tinoco > > On Mon, Oct 31, 2016 at 8:30 PM, Michael S. Tsirkin > wrote: > > On Mon, Oct 31, 2016 at 08:35:33AM -0200, Rafael David Tinoco wrote: > >> On Sun, Oct 30, 2016 at 5:26 PM, Michael S. Tsirkin > wrote: > >> > > >> > On Sat, Oct 22, 2016 at 07:00:41AM +0000, Rafael David Tinoco wrote: > >> > > Commit 31190ed7 added a migration blocker in vhost_dev_init() to > >> > > check if memfd would succeed. It is better if this blocker first > >> > > checks if vhost backend requires shared log. This will avoid a > >> > > situation where a blocker is added inappropriately (e.g. shared > >> > > log allocation fails when vhost backend doesn't support it). > >> > > >> > Sounds like a bugfix but I'm not sure. Can this part be split > >> > out in a patch by itself? > >> > >> Already sent some days ago (and pointed by Marc today). > >> > >> > > Commit: 35f9b6e added a fallback mechanism for systems not > supporting > >> > > memfd_create syscall (started being supported since 3.17). > >> > > > >> > > Backporting memfd_create might not be accepted for distros relying > >> > > on older kernels. Nowadays there is no way for security driver > >> > > to discover memfd filename to be created: /memfd-XXXXXX. > >> > > > >> > > Also, because vhost log file descriptors can be passed to other > >> > > processes, after discussion, we thought it is best to back mmap by > >> > > using files that can be placed into a specific directory: this > commit > >> > > creates "vhostlog" argv parameter for such purpose. This will allo= w > >> > > security drivers to operate on those files appropriately. > >> > > > >> > > Argv examples: > >> > > > >> > > -netdev tap,id=3Dnet0,vhost=3Don > >> > > -netdev tap,id=3Dnet0,vhost=3Don,vhostlog=3D/tmp/guest.log > >> > > -netdev tap,id=3Dnet0,vhost=3Don,vhostlog=3D/tmp > >> > > > >> > > For vhost backends supporting shared logs, if vhostlog is > non-existent, > >> > > or a directory, random files are going to be created in the > specified > >> > > directory (or, for non-existent, in tmpdir). If vhostlog is > specified, > >> > > the filepath is always used when allocating vhost log files. > >> > > >> > When vhostlog is not specified, can we just use memfd as we did? > >> > > >> > >> This was my approach on a "pastebin" example before this patch (in the > >> discussion thread we had). Problem goes back to when vhost log file > >> descriptor is shared with some vhost-user implementation - like the > >> interface allows to - and the security driver labelling issue. IMO, > >> yes, we could let vhostlog to specify a log file, and, if not > >> specified, assume memfd is ok to be used. > >> > >> Please let me know if you - and Marc - want me to keep using memfd. > >> I'll create the mmap-file tests and files in a different commit, like > >> Marc has asked for, and will propose the patch again by the end of > >> this week. > > > > I think that the best approach is to allow passing in the fd, > > not the file path. If not passed, use memfd. > > > > -- > > MST > > -- Marc-Andr=C3=A9 Lureau