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

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

[Kees Cook]

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>
---
 drivers/staging/greybus/loopback.c  | 14 ++++----------
 drivers/staging/greybus/operation.c |  7 +++----
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
index 08e255884206..045aaf81113a 100644
--- a/drivers/staging/greybus/loopback.c
+++ b/drivers/staging/greybus/loopback.c
@@ -572,16 +572,11 @@ 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);
 
-	op_async = gb_loopback_operation_find(id);
-	if (!op_async) {
-		pr_err("operation %d not found - time out ?\n", id);
-		return;
-	}
 	schedule_work(&op_async->work);
 }
 
@@ -631,8 +626,7 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
 	if (ret)
 		goto error;
 
-	setup_timer(&op_async->timer, gb_loopback_async_operation_timeout,
-			(unsigned long)operation->id);
+	timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0);
 	op_async->timer.expires = jiffies + gb->jiffy_timeout;
 	add_timer(&op_async->timer);
 
diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c
index 3023012808d9..ee4ba3f23bef 100644
--- a/drivers/staging/greybus/operation.c
+++ b/drivers/staging/greybus/operation.c
@@ -294,9 +294,9 @@ static void gb_operation_work(struct work_struct *work)
 	gb_operation_put(operation);
 }
 
-static void gb_operation_timeout(unsigned long arg)
+static void gb_operation_timeout(struct timer_list *t)
 {
-	struct gb_operation *operation = (void *)arg;
+	struct gb_operation *operation = from_timer(operation, t, timer);
 
 	if (gb_operation_result_set(operation, -ETIMEDOUT)) {
 		/*
@@ -541,8 +541,7 @@ gb_operation_create_common(struct gb_connection *connection, u8 type,
 			goto err_request;
 		}
 
-		setup_timer(&operation->timer, gb_operation_timeout,
-			    (unsigned long)operation);
+		timer_setup(&operation->timer, gb_operation_timeout, 0);
 	}
 
 	operation->flags = op_flags;
-- 
2.7.4


-- 
Kees Cook
Pixel Security

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

[Bryan O'Donoghue]

On 24/10/17 09:25, 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>
> ---
>   drivers/staging/greybus/loopback.c  | 14 ++++----------
>   drivers/staging/greybus/operation.c |  7 +++----
>   2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 08e255884206..045aaf81113a 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -572,16 +572,11 @@ 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);
>   
> -	op_async = gb_loopback_operation_find(id);
> -	if (!op_async) {
> -		pr_err("operation %d not found - time out ?\n", id);
> -		return;
> -	}

Hi Kees, you need to add

	gb_loopback_async_operation_get(op_async); when dropping the 
gb_loopback_operation_find() call here.

>   	schedule_work(&op_async->work);
>   }
>   
> @@ -631,8 +626,7 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
>   	if (ret)
>   		goto error;
>   
> -	setup_timer(&op_async->timer, gb_loopback_async_operation_timeout,
> -			(unsigned long)operation->id);
> +	timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0);
>   	op_async->timer.expires = jiffies + gb->jiffy_timeout;
>   	add_timer(&op_async->timer);
>   
> diff --git a/drivers/staging/greybus/operation.c b/drivers/staging/greybus/operation.c
> index 3023012808d9..ee4ba3f23bef 100644
> --- a/drivers/staging/greybus/operation.c
> +++ b/drivers/staging/greybus/operation.c
> @@ -294,9 +294,9 @@ static void gb_operation_work(struct work_struct *work)
>   	gb_operation_put(operation);
>   }
>   
> -static void gb_operation_timeout(unsigned long arg)
> +static void gb_operation_timeout(struct timer_list *t)
>   {
> -	struct gb_operation *operation = (void *)arg;
> +	struct gb_operation *operation = from_timer(operation, t, timer);
>   
>   	if (gb_operation_result_set(operation, -ETIMEDOUT)) {
>   		/*
> @@ -541,8 +541,7 @@ gb_operation_create_common(struct gb_connection *connection, u8 type,
>   			goto err_request;
>   		}
>   
> -		setup_timer(&operation->timer, gb_operation_timeout,
> -			    (unsigned long)operation);
> +		timer_setup(&operation->timer, gb_operation_timeout, 0);
>   	}
>   
>   	operation->flags = op_flags;
> 

Other than that, the rest looks good to me and you can add my

Reviewed-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>

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

[Bryan O'Donoghue]

On 24/10/17 10:35, Bryan O'Donoghue wrote:
> On 24/10/17 09:25, 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>
>> ---
>>   drivers/staging/greybus/loopback.c  | 14 ++++----------
>>   drivers/staging/greybus/operation.c |  7 +++----
>>   2 files changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/loopback.c 
>> b/drivers/staging/greybus/loopback.c
>> index 08e255884206..045aaf81113a 100644
>> --- a/drivers/staging/greybus/loopback.c
>> +++ b/drivers/staging/greybus/loopback.c
>> @@ -572,16 +572,11 @@ 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);
>> -    op_async = gb_loopback_operation_find(id);
>> -    if (!op_async) {
>> -        pr_err("operation %d not found - time out ?\n", id);
>> -        return;
>> -    }
> 
> Hi Kees, you need to add
> 
>      gb_loopback_async_operation_get(op_async); when dropping the 
> gb_loopback_operation_find() call here.

Actually:

	spin_lock_irqsave(&gb_dev.lock, flags);
	gb_loopback_async_operation_get(op_async);
         spin_unlock_irqrestore(&gb_dev.lock, flags);

---
bod

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

[Dan Carpenter]

On Tue, Oct 24, 2017 at 01:25:50AM -0700, 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>
> ---
>  drivers/staging/greybus/loopback.c  | 14 ++++----------
>  drivers/staging/greybus/operation.c |  7 +++----
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 08e255884206..045aaf81113a 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -572,16 +572,11 @@ 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);
>  

Since this one needs to be re-sent any way, could you do this instead of
breaking up the line?

	struct gb_loopback_async_operation *op_async;

	op_async = from_timer(op_async, t, timer);

regards,
dan carpenter

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

[Dan Carpenter]

Actually, on further reflection, I don't care.  However you write it is
fine.

regards,
dan carpenter

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

[Kees Cook]

On Tue, Oct 24, 2017 at 2:40 AM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On 24/10/17 10:35, Bryan O'Donoghue wrote:
>>
>> On 24/10/17 09:25, 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>
>>> ---
>>>   drivers/staging/greybus/loopback.c  | 14 ++++----------
>>>   drivers/staging/greybus/operation.c |  7 +++----
>>>   2 files changed, 7 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/staging/greybus/loopback.c
>>> b/drivers/staging/greybus/loopback.c
>>> index 08e255884206..045aaf81113a 100644
>>> --- a/drivers/staging/greybus/loopback.c
>>> +++ b/drivers/staging/greybus/loopback.c
>>> @@ -572,16 +572,11 @@ 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);
>>> -    op_async = gb_loopback_operation_find(id);
>>> -    if (!op_async) {
>>> -        pr_err("operation %d not found - time out ?\n", id);
>>> -        return;
>>> -    }
>>
>>
>> Hi Kees, you need to add
>>
>>      gb_loopback_async_operation_get(op_async); when dropping the
>> gb_loopback_operation_find() call here.
>
>
> Actually:
>
>         spin_lock_irqsave(&gb_dev.lock, flags);
>         gb_loopback_async_operation_get(op_async);
>         spin_unlock_irqrestore(&gb_dev.lock, flags);

Shouldn't the get/put follow the lifetime of the timer running
instead? It shouldnt' be possible to free the op_async while the timer
is still pending/running.

-Kees

-- 
Kees Cook
Pixel Security

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

[Bryan O'Donoghue]

On 24/10/17 13:47, Kees Cook wrote:
> On Tue, Oct 24, 2017 at 2:40 AM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
>> On 24/10/17 10:35, Bryan O'Donoghue wrote:
>>>
>>> On 24/10/17 09:25, 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>
>>>> ---
>>>>    drivers/staging/greybus/loopback.c  | 14 ++++----------
>>>>    drivers/staging/greybus/operation.c |  7 +++----
>>>>    2 files changed, 7 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/greybus/loopback.c
>>>> b/drivers/staging/greybus/loopback.c
>>>> index 08e255884206..045aaf81113a 100644
>>>> --- a/drivers/staging/greybus/loopback.c
>>>> +++ b/drivers/staging/greybus/loopback.c
>>>> @@ -572,16 +572,11 @@ 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);
>>>> -    op_async = gb_loopback_operation_find(id);
>>>> -    if (!op_async) {
>>>> -        pr_err("operation %d not found - time out ?\n", id);
>>>> -        return;
>>>> -    }
>>>
>>>
>>> Hi Kees, you need to add
>>>
>>>       gb_loopback_async_operation_get(op_async); when dropping the
>>> gb_loopback_operation_find() call here.
>>
>>
>> Actually:
>>
>>          spin_lock_irqsave(&gb_dev.lock, flags);
>>          gb_loopback_async_operation_get(op_async);
>>          spin_unlock_irqrestore(&gb_dev.lock, flags);
> 
> Shouldn't the get/put follow the lifetime of the timer running
> instead? It shouldnt' be possible to free the op_async while the timer
> is still pending/running.
> 
> -Kees
> 

The timeout timer runs for an operation that never completed but on the 
regular "everything is good" path you end up doing 
del_timer_sync(&op_async->timer); so the timer doesn't run.

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

[Kees Cook]

On Tue, Oct 24, 2017 at 5:52 AM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On 24/10/17 13:47, Kees Cook wrote:
>>
>> On Tue, Oct 24, 2017 at 2:40 AM, Bryan O'Donoghue
>> <pure.logic@nexus-software.ie> wrote:
>>>
>>> On 24/10/17 10:35, Bryan O'Donoghue wrote:
>>>>
>>>>
>>>> On 24/10/17 09:25, 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>
>>>>> ---
>>>>>    drivers/staging/greybus/loopback.c  | 14 ++++----------
>>>>>    drivers/staging/greybus/operation.c |  7 +++----
>>>>>    2 files changed, 7 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/greybus/loopback.c
>>>>> b/drivers/staging/greybus/loopback.c
>>>>> index 08e255884206..045aaf81113a 100644
>>>>> --- a/drivers/staging/greybus/loopback.c
>>>>> +++ b/drivers/staging/greybus/loopback.c
>>>>> @@ -572,16 +572,11 @@ 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);
>>>>> -    op_async = gb_loopback_operation_find(id);
>>>>> -    if (!op_async) {
>>>>> -        pr_err("operation %d not found - time out ?\n", id);
>>>>> -        return;
>>>>> -    }
>>>>
>>>>
>>>>
>>>> Hi Kees, you need to add
>>>>
>>>>       gb_loopback_async_operation_get(op_async); when dropping the
>>>> gb_loopback_operation_find() call here.
>>>
>>>
>>>
>>> Actually:
>>>
>>>          spin_lock_irqsave(&gb_dev.lock, flags);
>>>          gb_loopback_async_operation_get(op_async);
>>>          spin_unlock_irqrestore(&gb_dev.lock, flags);
>>
>> Shouldn't the get/put follow the lifetime of the timer running
>> instead? It shouldnt' be possible to free the op_async while the timer
>> is still pending/running.
>
> The timeout timer runs for an operation that never completed but on the
> regular "everything is good" path you end up doing
> del_timer_sync(&op_async->timer); so the timer doesn't run.

As far as I can tell, the get/put is already happening external to the timer:

gb_loopback_async_operation():
...
        op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
...
        INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
        kref_init(&op_async->kref);
...
        op_async->pending = true;
...
        ret = gb_operation_request_send(operation,
                                        gb_loopback_async_operation_callback,
                                        0,
                                        GFP_KERNEL);
...
        timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0);
        op_async->timer.expires = jiffies + gb->jiffy_timeout;
        add_timer(&op_async->timer);


gb_loopback_async_operation_callback(): (run by operation)
...
                op_async->pending = false;
                del_timer_sync(&op_async->timer);
                gb_loopback_async_operation_put(op_async);


gb_loopback_async_operation_work(): (scheduled by timer)
...
                op_async->pending = false;
                gb_loopback_async_operation_put(op_async);

(Doing a get/put in the timer callback itself shouldn't be happening:
it must already be pinned in memory for the callback to run.)

-Kees

-- 
Kees Cook
Pixel Security

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

[Bryan O'Donoghue]

On 24/10/17 14:14, Kees Cook wrote:
> On Tue, Oct 24, 2017 at 5:52 AM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
>> On 24/10/17 13:47, Kees Cook wrote:
>>>
>>> On Tue, Oct 24, 2017 at 2:40 AM, Bryan O'Donoghue
>>> <pure.logic@nexus-software.ie> wrote:
>>>>
>>>> On 24/10/17 10:35, Bryan O'Donoghue wrote:
>>>>>
>>>>>
>>>>> On 24/10/17 09:25, 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>
>>>>>> ---
>>>>>>     drivers/staging/greybus/loopback.c  | 14 ++++----------
>>>>>>     drivers/staging/greybus/operation.c |  7 +++----
>>>>>>     2 files changed, 7 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/greybus/loopback.c
>>>>>> b/drivers/staging/greybus/loopback.c
>>>>>> index 08e255884206..045aaf81113a 100644
>>>>>> --- a/drivers/staging/greybus/loopback.c
>>>>>> +++ b/drivers/staging/greybus/loopback.c
>>>>>> @@ -572,16 +572,11 @@ 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);
>>>>>> -    op_async = gb_loopback_operation_find(id);
>>>>>> -    if (!op_async) {
>>>>>> -        pr_err("operation %d not found - time out ?\n", id);
>>>>>> -        return;
>>>>>> -    }
>>>>>
>>>>>
>>>>>
>>>>> Hi Kees, you need to add
>>>>>
>>>>>        gb_loopback_async_operation_get(op_async); when dropping the
>>>>> gb_loopback_operation_find() call here.
>>>>
>>>>
>>>>
>>>> Actually:
>>>>
>>>>           spin_lock_irqsave(&gb_dev.lock, flags);
>>>>           gb_loopback_async_operation_get(op_async);
>>>>           spin_unlock_irqrestore(&gb_dev.lock, flags);
>>>
>>> Shouldn't the get/put follow the lifetime of the timer running
>>> instead? It shouldnt' be possible to free the op_async while the timer
>>> is still pending/running.
>>
>> The timeout timer runs for an operation that never completed but on the
>> regular "everything is good" path you end up doing
>> del_timer_sync(&op_async->timer); so the timer doesn't run.
> 
> As far as I can tell, the get/put is already happening external to the timer:
> 
> gb_loopback_async_operation():
> ...
>          op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
> ...
>          INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
>          kref_init(&op_async->kref);
> ...
>          op_async->pending = true;
> ...
>          ret = gb_operation_request_send(operation,
>                                          gb_loopback_async_operation_callback,
>                                          0,
>                                          GFP_KERNEL);
> ...
>          timer_setup(&op_async->timer, gb_loopback_async_operation_timeout, 0);
>          op_async->timer.expires = jiffies + gb->jiffy_timeout;
>          add_timer(&op_async->timer);
> 
> 
> gb_loopback_async_operation_callback(): (run by operation)
> ...
>                  op_async->pending = false;
>                  del_timer_sync(&op_async->timer);
>                  gb_loopback_async_operation_put(op_async);
> 
> 
> gb_loopback_async_operation_work(): (scheduled by timer)
> ...
>                  op_async->pending = false;
>                  gb_loopback_async_operation_put(op_async);
> 


> (Doing a get/put in the timer callback itself shouldn't be happening:
> it must already be pinned in memory for the callback to run.)

Both gb_loopback_async_operation_callback() and 
(gb_loopback_async_operation_timeout || 
gb_loopback_async_operation_work()) can run in parallel so the get in 
the timer callback means we can let the timer stuff free-run w/r to 
gb_loopback_async_operation_callback() - take a reference indicating we 
still want that object and then let gb_loopback_async_operation_work() run.

---
bod

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

[Kees Cook]

On Tue, Oct 24, 2017 at 6:30 AM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On 24/10/17 14:14, Kees Cook wrote:
>>
>> On Tue, Oct 24, 2017 at 5:52 AM, Bryan O'Donoghue
>> <pure.logic@nexus-software.ie> wrote:
>>>
>>> On 24/10/17 13:47, Kees Cook wrote:
>>>>
>>>>
>>>> On Tue, Oct 24, 2017 at 2:40 AM, Bryan O'Donoghue
>>>> <pure.logic@nexus-software.ie> wrote:
>>>>>
>>>>>
>>>>> On 24/10/17 10:35, Bryan O'Donoghue wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 24/10/17 09:25, 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>
>>>>>>> ---
>>>>>>>     drivers/staging/greybus/loopback.c  | 14 ++++----------
>>>>>>>     drivers/staging/greybus/operation.c |  7 +++----
>>>>>>>     2 files changed, 7 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/greybus/loopback.c
>>>>>>> b/drivers/staging/greybus/loopback.c
>>>>>>> index 08e255884206..045aaf81113a 100644
>>>>>>> --- a/drivers/staging/greybus/loopback.c
>>>>>>> +++ b/drivers/staging/greybus/loopback.c
>>>>>>> @@ -572,16 +572,11 @@ 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);
>>>>>>> -    op_async = gb_loopback_operation_find(id);
>>>>>>> -    if (!op_async) {
>>>>>>> -        pr_err("operation %d not found - time out ?\n", id);
>>>>>>> -        return;
>>>>>>> -    }
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Kees, you need to add
>>>>>>
>>>>>>        gb_loopback_async_operation_get(op_async); when dropping the
>>>>>> gb_loopback_operation_find() call here.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Actually:
>>>>>
>>>>>           spin_lock_irqsave(&gb_dev.lock, flags);
>>>>>           gb_loopback_async_operation_get(op_async);
>>>>>           spin_unlock_irqrestore(&gb_dev.lock, flags);
>>>>
>>>>
>>>> Shouldn't the get/put follow the lifetime of the timer running
>>>> instead? It shouldnt' be possible to free the op_async while the timer
>>>> is still pending/running.
>>>
>>>
>>> The timeout timer runs for an operation that never completed but on the
>>> regular "everything is good" path you end up doing
>>> del_timer_sync(&op_async->timer); so the timer doesn't run.
>>
>>
>> As far as I can tell, the get/put is already happening external to the
>> timer:
>>
>> gb_loopback_async_operation():
>> ...
>>          op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
>> ...
>>          INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
>>          kref_init(&op_async->kref);
>> ...
>>          op_async->pending = true;
>> ...
>>          ret = gb_operation_request_send(operation,
>>
>> gb_loopback_async_operation_callback,
>>                                          0,
>>                                          GFP_KERNEL);
>> ...
>>          timer_setup(&op_async->timer,
>> gb_loopback_async_operation_timeout, 0);
>>          op_async->timer.expires = jiffies + gb->jiffy_timeout;
>>          add_timer(&op_async->timer);
>>
>>
>> gb_loopback_async_operation_callback(): (run by operation)
>> ...
>>                  op_async->pending = false;
>>                  del_timer_sync(&op_async->timer);
>>                  gb_loopback_async_operation_put(op_async);
>>
>>
>> gb_loopback_async_operation_work(): (scheduled by timer)
>> ...
>>                  op_async->pending = false;
>>                  gb_loopback_async_operation_put(op_async);
>>
>
>
>> (Doing a get/put in the timer callback itself shouldn't be happening:
>> it must already be pinned in memory for the callback to run.)
>
>
> Both gb_loopback_async_operation_callback() and
> (gb_loopback_async_operation_timeout || gb_loopback_async_operation_work())
> can run in parallel so the get in the timer callback means we can let the
> timer stuff free-run w/r to gb_loopback_async_operation_callback() - take a
> reference indicating we still want that object and then let
> gb_loopback_async_operation_work() run.

Ah right, we're protecting the scheduled worker thread. I'll send a v2, thanks!

-Kees

-- 
Kees Cook
Pixel Security