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

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

[Nguyen Ngoc Thang]

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

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@vger.kernel.org
Reported-by: syzbot+a2e50fb40695811c2dab@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=a2e50fb40695811c2dab
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
v2: drop the now-unused err_put_active label (-Wunused-label, reported by the
    kernel test robot). The error path still falls through to it, so the
    behaviour is unchanged.

 drivers/greybus/operation.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
index 7e12ffb2dd60..968a4da5d1d4 100644
--- a/drivers/greybus/operation.c
+++ b/drivers/greybus/operation.c
@@ -766,18 +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. */
 	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_put_active:
+err_del_timer:
+	/* A timer that already fired has queued work which drops our references. */
+	if (timeout && !timer_delete_sync(&operation->timer))
+		return ret;
 	gb_operation_put_active(operation);
 err_put:
 	gb_operation_put(operation);
-- 
2.43.0

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

[Johan Hovold]

On Sun, Oct 18, 2026 at 10:13:40PM +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
> 
> 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.

Fixing this is going to be a bit more involved, for example, as you now
have to deal with the request message potentially being cancelled before
it has been submitted.

Sashiko also flagged this, and some other implications:

	https://sashiko.dev/#/patchset/20261017171743.24612-1-ngocthang2710.1999%40gmail.com

Johan

[PATCH v3] greybus: operation: close the race between completion and arming the timer

[Nguyen Ngoc Thang]

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 returns and drops the last reference, and the
operation is freed while the timer is 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

Keep sending the request first, so a request that is cancelled or times
out is always one that has been submitted. Instead, hold an extra
reference until the timer is armed, and delete the timer again if the
operation has completed by then, since the completion work could not
have seen it.

Fixes: dbec27298b0d ("staging: greybus: operation: add generic timeout support")
Cc: stable@vger.kernel.org
Reported-by: syzbot+a2e50fb40695811c2dab@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=a2e50fb40695811c2dab
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
Changes in v3 (after Johan's review):
- Do not reorder send and add_timer, so a cancelled or timed-out request
  has always been submitted. Hold an extra reference until the timer is
  armed and delete the timer if the operation already completed.
- Retested with the syzbot C reproducer in QEMU (DEBUG_OBJECTS_TIMERS):
  5 ODEBUG warnings per run before, none in 3/3 runs after.
- v1/v2 armed the timer before sending the message and are dropped.

 drivers/greybus/operation.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
index 7e12ffb2dd60..f3a65e693133 100644
--- a/drivers/greybus/operation.c
+++ b/drivers/greybus/operation.c
@@ -766,18 +766,28 @@ int gb_operation_request_send(struct gb_operation *operation,
 	if (ret)
 		goto err_put;
 
+	/* The completion can drop the other references before the timer is armed. */
+	gb_operation_get(operation);
+
 	ret = gb_message_send(operation->request, gfp);
 	if (ret)
-		goto err_put_active;
+		goto err_put_send;
 
 	if (timeout) {
 		operation->timer.expires = jiffies + msecs_to_jiffies(timeout);
 		add_timer(&operation->timer);
+
+		/* A completion that already ran did not see the timer. */
+		smp_mb();
+		if (READ_ONCE(operation->errno) != -EINPROGRESS)
+			timer_delete_sync(&operation->timer);
 	}
+	gb_operation_put(operation);
 
 	return 0;
 
-err_put_active:
+err_put_send:
+	gb_operation_put(operation);
 	gb_operation_put_active(operation);
 err_put:
 	gb_operation_put(operation);
-- 
2.43.0