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 <[email protected]>
> > Cc: "Bryan O'Donoghue" <[email protected]>
> > Cc: Johan Hovold <[email protected]>
> > Cc: Alex Elder <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > 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
From 1582683161350186831@xxx Mon Oct 30 11:51:21 +0000 2017
X-GM-THRID: 1582126742408023281
X-Gmail-Labels: Inbox,Category Forums
On 30/10/17 11:32, Johan Hovold wrote:
> 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.
Actually I wasn't clear if you wanted to to that yourself aswell as the
rest if it.
But sure I can do that conversion, it's on my list.
From 1582154961062066196@xxx Tue Oct 24 15:55:50 +0000 2017
X-GM-THRID: 1582126742408023281
X-Gmail-Labels: Inbox,Category Forums