Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup()

[Johan Hovold]

On Tue, Oct 24, 2017 at 04:54:59PM +0100, Bryan O'Donoghue wrote:
> On 24/10/17 15:49, Kees Cook wrote:
> > In preparation for unconditionally passing the struct timer_list pointer to
> > all timer callbacks, switch to using the new timer_setup() and from_timer()
> > to pass the timer pointer explicitly.
> > 
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: "Bryan O'Donoghue" <pure.logic@xxxxxxxxxxxxxxxxx>
> > Cc: Johan Hovold <johan@xxxxxxxxxx>
> > Cc: Alex Elder <elder@xxxxxxxxxx>
> > Cc: greybus-dev@xxxxxxxxxxxxxxxx
> > Cc: devel@xxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> > v2: Added back "get" in timer code, thanks to Bryan. :)
> > ---
> >   drivers/staging/greybus/loopback.c  | 19 +++++++++----------
> >   drivers/staging/greybus/operation.c |  7 +++----
> >   2 files changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> > index 08e255884206..1c0bafeb7ea5 100644
> > --- a/drivers/staging/greybus/loopback.c
> > +++ b/drivers/staging/greybus/loopback.c
> > @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work)
> >   	gb_loopback_async_operation_put(op_async);
> >   }
> >   
> > -static void gb_loopback_async_operation_timeout(unsigned long data)
> > +static void gb_loopback_async_operation_timeout(struct timer_list *t)
> >   {
> > -	struct gb_loopback_async_operation *op_async;
> > -	u16 id = data;
> > +	struct gb_loopback_async_operation *op_async =
> > +		from_timer(op_async, t, timer);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&gb_dev.lock, flags);
> > +	gb_loopback_async_operation_get(op_async);
> > +	spin_unlock_irqrestore(&gb_dev.lock, flags);
> 
> Damnit I'm just wrong (I hate that).
>
> The pointer can already have gone away by the time the timer runs - my 
> bad...

Hmm. Then something is really broken in this driver, you obviously must
never free the async operation which contains the timer while the timer
is active.

> see attached for update - with my Signed-off added.

The right thing to do here is to respin your patch from last year which
converts the loopback driver to use the timeout handling in greybus
core. Otherwise, I'm afraid you're not addressing the underlying bug.

Either way, Kees, please submit the operation.c conversion separately
from the loopback one, as the latter is non-trivial.

Thanks,
Johan