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