[Qemu-devel] Re: [v1 PATCH 3/3]: Convert v9fs_stat to threaded model.

[Venkateswararao Jujjuri (JV)]

On 3/16/2011 10:10 AM, Stefan Hajnoczi wrote:
> On Wed, Mar 16, 2011 at 2:33 PM, Venkateswararao Jujjuri (JV)
> <address@hidden> wrote:
>> On 3/16/2011 3:23 AM, Stefan Hajnoczi wrote:
>>> On Tue, Mar 15, 2011 at 10:39 AM, Arun R Bharadwaj
>>> <address@hidden> 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