Re: [PULL 1/3] qemu-coroutine-sleep: introduce qemu_co_sleep_wake

[Eric Blake]

On 11/8/19 12:42 PM, Peter Maydell wrote:
> On Wed, 23 Oct 2019 at 03:04, Eric Blake <address@hidden> wrote:
> > From: Vladimir Sementsov-Ogievskiy <address@hidden>
> >
> > Introduce a function to gracefully wake a coroutine sleeping in
> > qemu_co_sleep_ns().
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > Reviewed-by: Kevin Wolf <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > Message-Id: <address@hidden>
> > Signed-off-by: Eric Blake <address@hidden>
>
> Hi; Coverity reports an issue in this patch (CID 1406474):

> > - * Yield the coroutine for a given duration
> > + * Yield the coroutine for a given duration. During this yield, @sleep_state
> > + * (if not NULL) is set to an opaque pointer, which may be used for
> > + * qemu_co_sleep_wake(). Be careful, the pointer is set back to zero when the
> > + * timer fires. Don't save the obtained value to other variables and don't call
> > + * qemu_co_sleep_wake from another aio context.
> >   */
> > -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);

Here, we document the rules on what will happen to *sleep_state (in 
particular, since we store a stack local variable into it, the caller 
must not leak it further, and it will be reclaimed back to zero before 
this function finally finishes).

> > -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns)
> > +void coroutine_fn qemu_co_sleep_ns_wakeable(QEMUClockType type, int64_t ns,
> > +                                            QemuCoSleepState **sleep_state)
> >   {
> >       AioContext *ctx = qemu_get_current_aio_context();
> > -    QEMUTimer *ts;
> > -    Coroutine *co = qemu_coroutine_self();
> > +    QemuCoSleepState state = {
> > +        .co = qemu_coroutine_self(),
> > +        .ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, &state),
> > +        .user_state_pointer = sleep_state,
> > +    };
>
> Here 'state' is a variable on the stack...
>
> > -    const char *scheduled = atomic_cmpxchg(&co->scheduled, NULL, __func__);
> > +    const char *scheduled = atomic_cmpxchg(&state.co->scheduled, NULL,
> > +                                           qemu_co_sleep_ns__scheduled);
> >       if (scheduled) {
> >           fprintf(stderr,
> >                   "%s: Co-routine was already scheduled in '%s'\n",
> >                   __func__, scheduled);
> >           abort();
> >       }
> > -    ts = aio_timer_new(ctx, type, SCALE_NS, co_sleep_cb, co);
> > -    timer_mod(ts, qemu_clock_get_ns(type) + ns);
> > +
> > +    if (sleep_state) {
> > +        *sleep_state = &state;
>
> ...here we save a pointer to it into *sleep_state which was
> passed to us by the caller...
>
> > +    }
> > +    timer_mod(state.ts, qemu_clock_get_ns(type) + ns);
> >       qemu_coroutine_yield();

And here is where we yield, which is the only point at which the caller 
will see a non-zero value in *sleep_state in the first place, at which 
point the caller must follow the rules we document.

> > -    timer_del(ts);
> > -    timer_free(ts);
> > +    timer_free(state.ts);
>
> ...and here we return from this function, which means 'state'
> is no longer in valid memory, but the caller has still been
> given a pointer to it.
>
> >   }
>
> Is this just Coverity getting confused by our coroutine code?
> (I certainly find it confusing...)

Yes, Coverity is unable to see that we require that the caller MUST obey 
rules with the use of it's access to sleep_state. However, it might be 
possible after the yield to assert(!sleep_state || *sleep_state == NULL) 
to prove that the caller's use of our temporary leak of a stack variable 
was solely to get this coroutine to resume from yield, and that the 
resumption cleared it, so that the end of the function is not leaking 
anything.

I guess it's worth experimenting to see if such a patch would silence 
Coverity without breaking the code...

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org