On Thu, Mar 19, 2026 at 12:20:49PM -0400, Damien Riégel wrote:
If a user writes to the chardev after disconnect has been called, the kernel panics with the following trace (with CONFIG_INIT_ON_FREE_DEFAULT_ON=y):
BUG: kernel NULL pointer dereference, address: 0000000000000218 ... Call Trace: <TASK> gb_operation_create_common+0x61/0x180 gb_operation_create_flags+0x28/0xa0 gb_operation_sync_timeout+0x6f/0x100 raw_write+0x7b/0xc7 [gb_raw] vfs_write+0xcf/0x420 ? task_mm_cid_work+0x136/0x220 ksys_write+0x63/0xe0 do_syscall_64+0xa4/0x290 entry_SYSCALL_64_after_hwframe+0x77/0x7fDisconnect calls gb_connection_destroy, which ends up freeing the connection object. When gb_operation_sync is called in the write file operations, its gets a freed connection as parameter and the kernel panics.
The gb_connection_destroy cannot be moved out of the disconnect function, as the Greybus subsystem expect all connections belonging to a bundle to be destroyed when disconnect returns.
To prevent this bug, use a rw lock to synchronize access between write and disconnect. This guarantees that in the write function raw->connection is either a valid object or a NULL pointer.
You forgot to update this last sentence after you switched to a bool flag.
Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Signed-off-by: Damien Riégel damien.riegel@silabs.com
Changes in v2:
- trim down trace in commit message to keep only the essential part
- convert the mutex that protected the connection to a rw_semaphore
- use a "connected" flag instead of relying on the connection pointer being NULL or not
drivers/staging/greybus/raw.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 6da878e4339..57bf5032280 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,6 +21,8 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock;
- struct rw_semaphore disconnect_lock; /* Synchronize access to connection */
Just skip the comment (and ignore checkpatch).
- bool connected;
Please use a "disconnected" flag instead (and leave it set to false until disconnect() is called) which is the common way to handle this.
struct cdev cdev; struct device dev; }; @@ -124,7 +126,6 @@ static int gb_raw_request_handler(struct gb_operation *op) static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) {
- struct gb_connection *connection = raw->connection; struct gb_raw_send_request *request; int retval;
@@ -139,9 +140,18 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) request->len = cpu_to_le32(len);
- retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND,
- down_read(&raw->disconnect_lock);
- if (!raw->connected) {
retval = -ENODEV;goto exit;- }
I think it may be preferred to move the disconnected check to raw_write() to avoid allocating memory and copying data for a bundle (connection) that's already gone.
- retval = gb_operation_sync(raw->connection, GB_RAW_TYPE_SEND, request, len + sizeof(*request), NULL, 0);
+exit:
- up_read(&raw->disconnect_lock);
kfree(request); return retval; @@ -199,6 +209,7 @@ static int gb_raw_probe(struct gb_bundle *bundle, INIT_LIST_HEAD(&raw->list); mutex_init(&raw->list_lock);
- init_rwsem(&raw->disconnect_lock);
raw->connection = connection; raw->dev.parent = &connection->bundle->dev; @@ -210,6 +221,8 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (retval) goto error_connection_destroy;
- raw->connected = true;
No need to initialise after you invert the flag.
Johan