From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59974) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8Pr7-0002FH-KQ for qemu-devel@nongnu.org; Fri, 26 Jun 2015 05:23:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8Pr3-0005CN-B0 for qemu-devel@nongnu.org; Fri, 26 Jun 2015 05:23:29 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:58359 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8Pr3-0005B9-0H for qemu-devel@nongnu.org; Fri, 26 Jun 2015 05:23:25 -0400 Message-ID: <558D1A06.9060608@kamp.de> Date: Fri, 26 Jun 2015 11:23:18 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1435047135-31647-1-git-send-email-pl@kamp.de> <20150625131806.GG4419@stefanha-thinkpad.redhat.com> <558C0196.1050208@kamp.de> <20150626091435.GC15457@stefanha-thinkpad.redhat.com> In-Reply-To: <20150626091435.GC15457@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] block/nfs: add support for setting debug level List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, ronniesahlberg@gmail.com, qemu-devel@nongnu.org, qemu-block@nongnu.org Am 26.06.2015 um 11:14 schrieb Stefan Hajnoczi: > On Thu, Jun 25, 2015 at 03:26:46PM +0200, Peter Lieven wrote: >> Am 25.06.2015 um 15:18 schrieb Stefan Hajnoczi: >>> On Tue, Jun 23, 2015 at 10:12:15AM +0200, Peter Lieven wrote: >>>> upcoming libnfs versions will support logging debug messages. Add >>>> support for it in qemu through an URL parameter. >>>> >>>> Signed-off-by: Peter Lieven >>>> --- >>>> block/nfs.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/block/nfs.c b/block/nfs.c >>>> index ca9e24e..f7388a3 100644 >>>> --- a/block/nfs.c >>>> +++ b/block/nfs.c >>>> @@ -329,6 +329,10 @@ static int64_t nfs_client_open(NFSClient *client, const char *filename, >>>> } else if (!strcmp(qp->p[i].name, "readahead")) { >>>> nfs_set_readahead(client->context, val); >>>> #endif >>>> +#ifdef LIBNFS_FEATURE_DEBUG >>>> + } else if (!strcmp(qp->p[i].name, "debug")) { >>>> + nfs_set_debug(client->context, val); >>>> +#endif >>>> } else { >>>> error_setg(errp, "Unknown NFS parameter name: %s", >>>> qp->p[i].name); >>> Untrusted users may be able to set these options since they are encoded >>> in the URI. I'm imagining a hosting or cloud scenario like OpenStack. >>> >>> A verbose debug level spams stderr and could consume a lot of disk >>> space. >>> >>> (The uid and gid options are probably okay since the NFS server cannot >>> trust the uid/gid coming from QEMU anyway.) >>> >>> I think we can merge this patch for QEMU 2.4 but I'd like to have a >>> discussion about the security risk of encoding libnfs options in the >>> URI. >>> >>> CCed Eric Blake in case libvirt is affected. >>> >>> Has anyone thought about this and what are the rules? >> Good point. In general I think there should be some kind of sanitization of the parameters >> before they are passed on to Qemu. In our use case the user cannot pass any kind of URIs himself, >> but this might be different in other backends. The readahead value is as dangerous as well >> if not sanitized. >> >> I had a discussion with Ronnie in the past if we should encode parameters in the URI or via environment >> like it is done in libiscsi. If I remember correctly we came up with the URI parameters for better usability, >> but hadn't attack scenarios in mind. >> >> I am also open to only allow uncritical parameters in the URI and pass others via a new -nfs cmdline option. >> Or limit max readahead and max debug level settable via URI. > I'd feel safer if the option was in runtime_opts instead. The the > management tool has to pass them explicitly and the end user cannot > influence them via the URI. > > If an option is needed both at open and create time, then it must also > be parsed from nfs_file_create() opts. Ok, I will send a patch that follows this approach. And also a second one to limit the readahead size to a reasonable value. Peter