Re: [PATCH] greybus: operation: arm the timeout timer before sending the request

[Greg Kroah-Hartman]

On Sun, Oct 18, 2026 at 12:17:43AM +0700, Nguyen Ngoc Thang wrote:
> gb_operation_request_send() sends the request message and only then arms
> the operation timer. The host driver can complete the operation before
> add_timer() runs, e.g. when the transfer fails right away. The completion
> work then calls timer_delete_sync() on a timer that is not armed yet,
> the synchronous caller wakes up and drops the last reference, and
> the operation is freed while the timer is still pending:
> 
>   ODEBUG: free active (active state 0) object type: timer_list
>           hint: gb_operation_timeout+0x0/0xc0
>   WARNING: lib/debugobjects.c:629 at debug_print_object+0x198/0x2b0
>    kmem_cache_free+0xfd/0x6d0
>    gb_operation_sync_timeout+0x1c7/0x270
>    gb_svc_process_deferred_request+0x5f3/0x1150

How was this generated and tested?

> Arm the timer before the message is sent. If sending fails, delete the
> timer again. If it has already fired, its work is queued and drops the
> active and operation references itself, so the error path must not drop
> them a second time.
> 
> Fixes: dbec27298b0d ("staging: greybus: operation: add generic timeout support")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: syzbot+a2e50fb40695811c2dab@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=a2e50fb40695811c2dab
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
> ---
>  drivers/greybus/operation.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
> index 7e12ffb2dd60..19b0cdeca498 100644
> --- a/drivers/greybus/operation.c
> +++ b/drivers/greybus/operation.c
> @@ -766,17 +766,22 @@ int gb_operation_request_send(struct gb_operation *operation,
>  	if (ret)
>  		goto err_put;
>  
> -	ret = gb_message_send(operation->request, gfp);
> -	if (ret)
> -		goto err_put_active;
> -
> +	/* Arm before sending, as the response may complete the operation at once. */

"Arm" what?

>  	if (timeout) {
>  		operation->timer.expires = jiffies + msecs_to_jiffies(timeout);
>  		add_timer(&operation->timer);
>  	}
>  
> +	ret = gb_message_send(operation->request, gfp);
> +	if (ret)
> +		goto err_del_timer;
> +
>  	return 0;
>  
> +err_del_timer:
> +	/* A timer that already fired has queued work which drops our references. */

Why is this comment needed?

> +	if (timeout && !timer_delete_sync(&operation->timer))
> +		return ret;

Did you just leak this:

>  err_put_active:
>  	gb_operation_put_active(operation);

And again, how was this tested?

thanks,

greg k-h