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@linuxfoundation.org Cc: "Bryan O'Donoghue" pure.logic@nexus-software.ie Cc: Johan Hovold johan@kernel.org Cc: Alex Elder elder@kernel.org Cc: greybus-dev@lists.linaro.org Cc: devel@driverdev.osuosl.org Signed-off-by: Kees Cook keescook@chromium.org
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