On Wed, Mar 11, 2026 at 05:25:11PM -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):
[ 83.828726] BUG: kernel NULL pointer dereference, address: 0000000000000218
Please trim this oops too. The timestamps are not needed either (in either patch).
[ 83.835259] Call Trace: [ 83.835983] <TASK> [ 83.836362] gb_operation_create_common+0x61/0x180 [ 83.836653] gb_operation_create_flags+0x28/0xa0 [ 83.836912] gb_operation_sync_timeout+0x6f/0x100 [ 83.837162] raw_write+0x7b/0xc7 [gb_raw] [ 83.837460] vfs_write+0xcf/0x420
Disconnect 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 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.
Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Signed-off-by: Damien Riégel damien.riegel@silabs.com
resend: added linux-staging as Cc, this list was not part of the first submission.
drivers/staging/greybus/raw.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index b92214f97e3..aa4086ff397 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,6 +21,7 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock;
- struct mutex write_lock; /* Synchronize access to connection */
This works here, but I think it would be better to generalise this so that it could be used for possible future ioctl() and read() too.
For that you can use an rw semaphore and name it something like disconnect_lock. And possibly use a dedicated boolean flag for the disconnected state.
struct cdev cdev; struct device dev; }; @@ -124,8 +125,8 @@ 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;
- struct gb_connection *connection; int retval;
request = kmalloc(len + sizeof(*request), GFP_KERNEL); @@ -139,9 +140,15 @@ 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,
request, len + sizeof(*request),NULL, 0);
- mutex_lock(&raw->write_lock);
Then this would be a read lock.
And as part of this or a follow-on patch you also take a read lock in read so that user space can be notified that the device is gone rather than trying to read the empty buffers indefinitely.
- retval = -ENODEV;
Please check raw->disconnected (or !raw_connected) here and bail out after setting retval instead.
- connection = raw->connection;
- if (connection)
retval = gb_operation_sync(connection, GB_RAW_TYPE_SEND,request, len + sizeof(*request),NULL, 0);- mutex_unlock(&raw->write_lock);
kfree(request); return retval;
@@ -238,9 +246,9 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *temp; cdev_device_del(&raw->cdev, &raw->dev);
- gb_connection_disable(connection); ida_free(&minors, MINOR(raw->dev.devt));
- gb_connection_destroy(connection);
- gb_connection_disable(connection);
mutex_lock(&raw->list_lock); list_for_each_entry_safe(raw_data, temp, &raw->list, entry) { @@ -248,6 +256,12 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock);
- mutex_lock(&raw->write_lock);
- raw->connection = NULL;
- gb_connection_destroy(connection);
- mutex_unlock(&raw->write_lock);
Then this would be a write lock setting the disconnected flag, and that can be done before freeing the data buffers (or before disabling the connection).
- put_device(&raw->dev);
}
Johan