From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53944 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q04nO-0005no-LW for qemu-devel@nongnu.org; Thu, 17 Mar 2011 00:26:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q04nN-0000DN-Aw for qemu-devel@nongnu.org; Thu, 17 Mar 2011 00:26:46 -0400 Received: from e6.ny.us.ibm.com ([32.97.182.146]:56066) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q04nN-0000DD-8O for qemu-devel@nongnu.org; Thu, 17 Mar 2011 00:26:45 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e6.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2H42Q00003090 for ; Thu, 17 Mar 2011 00:02:26 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id E5C5838C8038 for ; Thu, 17 Mar 2011 00:26:38 -0400 (EDT) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2H4Qfhv128046 for ; Thu, 17 Mar 2011 00:26:42 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2H4Qfes026602 for ; Wed, 16 Mar 2011 22:26:41 -0600 Message-ID: <4D818D7C.2080705@linux.vnet.ibm.com> Date: Wed, 16 Mar 2011 21:26:36 -0700 From: "Venkateswararao Jujjuri (JV)" MIME-Version: 1.0 References: <20110315103453.GA23922@linux.vnet.ibm.com> <20110315103901.GD23922@linux.vnet.ibm.com> <4D80CA34.6040205@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: arun@linux.vnet.ibm.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, aneesh.kumar@linux.vnet.ibm.com On 3/16/2011 10:10 AM, Stefan Hajnoczi wrote: > On Wed, Mar 16, 2011 at 2:33 PM, Venkateswararao Jujjuri (JV) > wrote: >> On 3/16/2011 3:23 AM, Stefan Hajnoczi wrote: >>> On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj >>> wrote: >>>> -static void v9fs_stat_post_lstat(V9fsState *s, V9fsStatState *vs, int err) >>>> +static void v9fs_stat_post_lstat(void *opaque) >>>> { >>>> - if (err == -1) { >>>> - err = -errno; >>>> + V9fsStatState *vs = (V9fsStatState *)opaque; >>> >>> No need to cast void* in C. >>> >>>> + if (vs->err == -1) { >>>> + vs->err = -(vs->v9fs_errno); >>> >>> How about the thread worker function puts the -errno into a vs->ret field: >>> >>> static void v9fs_stat_do_lstat(V9fsRequest *request) >>> { >>> V9fsStatState *vs = container_of(request, V9fsStatState, request); >>> >>> vs->ret = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf); >>> if (vs->ret != 0) { >>> vs->ret = -errno; >>> } >>> } >>> >>> Then v9fs_stat_post_lstat() can use vs->ret directly and does not need >>> to juggle around the two fields, vs->err and vs->v9fs_errno. >>> >>>> goto out; >>>> } >>>> >>>> - err = stat_to_v9stat(s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat); >>>> - if (err) { >>>> + vs->err = stat_to_v9stat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf, &vs->v9stat); >>> >>> This function can block in v9fs_do_readlink(). Needs to be done >>> asynchronously to avoid blocking QEMU. >>> >>>> + if (vs->err) { >>>> goto out; >>>> } >>>> vs->offset += pdu_marshal(vs->pdu, vs->offset, "wS", 0, &vs->v9stat); >>>> - err = vs->offset; >>>> + vs->err = vs->offset; >>>> >>>> out: >>>> - complete_pdu(s, vs->pdu, err); >>>> + complete_pdu(vs->s, vs->pdu, vs->err); >>>> v9fs_stat_free(&vs->v9stat); >>>> qemu_free(vs); >>>> } >>>> >>>> +static void v9fs_stat_do_lstat(V9fsRequest *request) >>>> +{ >>>> + V9fsStatState *vs = container_of(request, V9fsStatState, request); >>> >>> Nice. Could container_of() be used for v9fs_post_lstat() too? I'm >>> suggesting making post op functions take the V9fsRequest* instead of a >>> void* opaque pointer. >>> >>>> + >>>> + vs->err = v9fs_do_lstat(vs->s, &vs->fidp->fsmap.path, &vs->stbuf); >>> >>> This is not threadsafe since rpath still uses a static buffer in >>> qemu.git. Please ensure that rpath() is thread-safe before pushing >>> this patch. >> >> There is another patch on the internal list to make rpath thread safe. >> >>> >>>> + vs->v9fs_errno = errno; >>>> +} >>>> + >>>> static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) >>>> { >>>> int32_t fid; >>>> @@ -1487,6 +1496,10 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) >>>> vs = qemu_malloc(sizeof(*vs)); >>>> vs->pdu = pdu; >>>> vs->offset = 7; >>>> + vs->s = s; >>>> + vs->request.func = v9fs_stat_do_lstat; >>>> + vs->request.post_op.func = v9fs_stat_post_lstat; >>>> + vs->request.post_op.arg = vs; >>>> >>>> memset(&vs->v9stat, 0, sizeof(vs->v9stat)); >>>> >>>> @@ -1498,8 +1511,11 @@ static void v9fs_stat(V9fsState *s, V9fsPDU *pdu) >>>> goto out; >>>> } >>>> >>>> + /* >>>> err = v9fs_do_lstat(s, &vs->fidp->fsmap.path, &vs->stbuf); >>>> v9fs_stat_post_lstat(s, vs, err); >>>> + */ >>> >>> Please remove unused code, it quickly becomes out-of-date and confuses readers. >>> >>>> + v9fs_qemu_submit_request(&vs->request); >>> >>> What happens when another PDU is handled next that uses the same fid? >>> The worst case is if the client sends TCLUNK and fid is freed while >>> the worker thread and later the post op still access the memory. >>> There needs to be some kind of guard (like a reference count) to >>> prevent this. >> >> As per the protocol this should not happen. Client is the controls the fid, >> and the fid is created or destroyed per the directive of the client. >> It should not send clunk until the response is received on that fid >> based operation(if there is any). > > Unfortunately it is still possible for a guest to do it. The model > for emulated hardware is that the guest is untrusted and we cannot > allow things to crash. Well, it can happen only if the guest OS is hacked...and the worst thing can happen is guest goes down. I am not sure how it is different from a bare metal system.. > > It's really important for everyone to keep this in mind otherwise > security vulnerabilities will be introduced for use cases like hosting > and cloud where the guest really cannot be trusted. > > An easy fix is to mark a fid busy and reject requests that mess with > it before the existing request has been processed. > > Stefan