* [PATCH 2/2] binder: use guards for plain mutex- and spinlock-protected sections
2025-06-26 7:30 [PATCH 1/2] binder: use kstrdup() in binderfs_binder_device_create() Dmitry Antipov
@ 2025-06-26 7:30 ` Dmitry Antipov
2025-07-15 3:51 ` Carlos Llamas
2025-07-15 7:38 ` Alice Ryhl
2025-06-26 17:30 ` [PATCH 1/2] binder: use kstrdup() in binderfs_binder_device_create() Tiffany Yang
` (2 subsequent siblings)
3 siblings, 2 replies; 7+ messages in thread
From: Dmitry Antipov @ 2025-06-26 7:30 UTC (permalink / raw)
To: Carlos Llamas
Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
Tiffany Y . Yang, linux-kernel, Dmitry Antipov
Use 'guard(mutex)' and 'guard(spinlock)' for plain (i.e. non-scoped)
mutex- and spinlock-protected sections, respectively, thus making
locking a bit simpler. Briefly tested with 'stress-ng --binderfs'.
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
drivers/android/binder.c | 33 +++++++++++----------------------
drivers/android/binder_alloc.c | 14 ++++----------
drivers/android/binder_alloc.h | 8 ++------
3 files changed, 17 insertions(+), 38 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c463ca4a8fff..b8a6a513e9b5 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1585,11 +1585,10 @@ static struct binder_thread *binder_get_txn_from(
{
struct binder_thread *from;
- spin_lock(&t->lock);
+ guard(spinlock)(&t->lock);
from = t->from;
if (from)
atomic_inc(&from->tmp_ref);
- spin_unlock(&t->lock);
return from;
}
@@ -5445,32 +5444,28 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp,
struct binder_node *new_node;
kuid_t curr_euid = current_euid();
- mutex_lock(&context->context_mgr_node_lock);
+ guard(mutex)(&context->context_mgr_node_lock);
if (context->binder_context_mgr_node) {
pr_err("BINDER_SET_CONTEXT_MGR already set\n");
- ret = -EBUSY;
- goto out;
+ return -EBUSY;
}
ret = security_binder_set_context_mgr(proc->cred);
if (ret < 0)
- goto out;
+ return ret;
if (uid_valid(context->binder_context_mgr_uid)) {
if (!uid_eq(context->binder_context_mgr_uid, curr_euid)) {
pr_err("BINDER_SET_CONTEXT_MGR bad uid %d != %d\n",
from_kuid(&init_user_ns, curr_euid),
from_kuid(&init_user_ns,
context->binder_context_mgr_uid));
- ret = -EPERM;
- goto out;
+ return -EPERM;
}
} else {
context->binder_context_mgr_uid = curr_euid;
}
new_node = binder_new_node(proc, fbo);
- if (!new_node) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!new_node)
+ return -ENOMEM;
binder_node_lock(new_node);
new_node->local_weak_refs++;
new_node->local_strong_refs++;
@@ -5479,8 +5474,6 @@ static int binder_ioctl_set_ctx_mgr(struct file *filp,
context->binder_context_mgr_node = new_node;
binder_node_unlock(new_node);
binder_put_node(new_node);
-out:
- mutex_unlock(&context->context_mgr_node_lock);
return ret;
}
@@ -6322,14 +6315,13 @@ static DECLARE_WORK(binder_deferred_work, binder_deferred_func);
static void
binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer)
{
- mutex_lock(&binder_deferred_lock);
+ guard(mutex)(&binder_deferred_lock);
proc->deferred_work |= defer;
if (hlist_unhashed(&proc->deferred_work_node)) {
hlist_add_head(&proc->deferred_work_node,
&binder_deferred_list);
schedule_work(&binder_deferred_work);
}
- mutex_unlock(&binder_deferred_lock);
}
static void print_binder_transaction_ilocked(struct seq_file *m,
@@ -6871,14 +6863,13 @@ static int proc_show(struct seq_file *m, void *unused)
struct binder_proc *itr;
int pid = (unsigned long)m->private;
- mutex_lock(&binder_procs_lock);
+ guard(mutex)(&binder_procs_lock);
hlist_for_each_entry(itr, &binder_procs, proc_node) {
if (itr->pid == pid) {
seq_puts(m, "binder proc state:\n");
print_binder_proc(m, itr, true, false);
}
}
- mutex_unlock(&binder_procs_lock);
return 0;
}
@@ -6996,16 +6987,14 @@ const struct binder_debugfs_entry binder_debugfs_entries[] = {
void binder_add_device(struct binder_device *device)
{
- spin_lock(&binder_devices_lock);
+ guard(spinlock)(&binder_devices_lock);
hlist_add_head(&device->hlist, &binder_devices);
- spin_unlock(&binder_devices_lock);
}
void binder_remove_device(struct binder_device *device)
{
- spin_lock(&binder_devices_lock);
+ guard(spinlock)(&binder_devices_lock);
hlist_del_init(&device->hlist);
- spin_unlock(&binder_devices_lock);
}
static int __init init_binder_device(const char *name)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index fcfaf1b899c8..a0a7cb58fc05 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -167,12 +167,8 @@ static struct binder_buffer *binder_alloc_prepare_to_free_locked(
struct binder_buffer *binder_alloc_prepare_to_free(struct binder_alloc *alloc,
unsigned long user_ptr)
{
- struct binder_buffer *buffer;
-
- mutex_lock(&alloc->mutex);
- buffer = binder_alloc_prepare_to_free_locked(alloc, user_ptr);
- mutex_unlock(&alloc->mutex);
- return buffer;
+ guard(mutex)(&alloc->mutex);
+ return binder_alloc_prepare_to_free_locked(alloc, user_ptr);
}
static inline void
@@ -1043,7 +1039,7 @@ void binder_alloc_print_allocated(struct seq_file *m,
struct binder_buffer *buffer;
struct rb_node *n;
- mutex_lock(&alloc->mutex);
+ guard(mutex)(&alloc->mutex);
for (n = rb_first(&alloc->allocated_buffers); n; n = rb_next(n)) {
buffer = rb_entry(n, struct binder_buffer, rb_node);
seq_printf(m, " buffer %d: %lx size %zd:%zd:%zd %s\n",
@@ -1053,7 +1049,6 @@ void binder_alloc_print_allocated(struct seq_file *m,
buffer->extra_buffers_size,
buffer->transaction ? "active" : "delivered");
}
- mutex_unlock(&alloc->mutex);
}
/**
@@ -1102,10 +1097,9 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc)
struct rb_node *n;
int count = 0;
- mutex_lock(&alloc->mutex);
+ guard(mutex)(&alloc->mutex);
for (n = rb_first(&alloc->allocated_buffers); n != NULL; n = rb_next(n))
count++;
- mutex_unlock(&alloc->mutex);
return count;
}
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index feecd7414241..a9d5f3169e12 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -160,12 +160,8 @@ void binder_alloc_print_pages(struct seq_file *m,
static inline size_t
binder_alloc_get_free_async_space(struct binder_alloc *alloc)
{
- size_t free_async_space;
-
- mutex_lock(&alloc->mutex);
- free_async_space = alloc->free_async_space;
- mutex_unlock(&alloc->mutex);
- return free_async_space;
+ guard(mutex)(&alloc->mutex);
+ return alloc->free_async_space;
}
unsigned long
--
2.50.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] binder: use kstrdup() in binderfs_binder_device_create()
2025-06-26 7:30 [PATCH 1/2] binder: use kstrdup() in binderfs_binder_device_create() Dmitry Antipov
2025-06-26 7:30 ` [PATCH 2/2] binder: use guards for plain mutex- and spinlock-protected sections Dmitry Antipov
@ 2025-06-26 17:30 ` Tiffany Yang
2025-07-15 3:45 ` Carlos Llamas
2025-07-15 7:32 ` Alice Ryhl
3 siblings, 0 replies; 7+ messages in thread
From: Tiffany Yang @ 2025-06-26 17:30 UTC (permalink / raw)
To: Dmitry Antipov
Cc: Carlos Llamas, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, linux-kernel
Dmitry Antipov <dmantipov@yandex.ru> writes:
> In 'binderfs_binder_device_create()', use 'kstrdup()' to copy the
> newly created device's name, thus making the former a bit simpler.
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> drivers/android/binderfs.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 024275dbfdd8..4f827152d18e 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -117,7 +117,6 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
> struct dentry *dentry, *root;
> struct binder_device *device;
> char *name = NULL;
> - size_t name_len;
> struct inode *inode = NULL;
> struct super_block *sb = ref_inode->i_sb;
> struct binderfs_info *info = sb->s_fs_info;
> @@ -161,9 +160,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
> inode->i_gid = info->root_gid;
>
> req->name[BINDERFS_MAX_NAME] = '\0'; /* NUL-terminate */
> - name_len = strlen(req->name);
> - /* Make sure to include terminating NUL byte */
> - name = kmemdup(req->name, name_len + 1, GFP_KERNEL);
> + name = kstrdup(req->name, GFP_KERNEL);
> if (!name)
> goto err;
Reviewed-by: Tiffany Y. Yang <ynaffit@google.com>
--
Tiffany Y. Yang
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] binder: use kstrdup() in binderfs_binder_device_create()
2025-06-26 7:30 [PATCH 1/2] binder: use kstrdup() in binderfs_binder_device_create() Dmitry Antipov
2025-06-26 7:30 ` [PATCH 2/2] binder: use guards for plain mutex- and spinlock-protected sections Dmitry Antipov
2025-06-26 17:30 ` [PATCH 1/2] binder: use kstrdup() in binderfs_binder_device_create() Tiffany Yang
@ 2025-07-15 3:45 ` Carlos Llamas
2025-07-15 7:32 ` Alice Ryhl
3 siblings, 0 replies; 7+ messages in thread
From: Carlos Llamas @ 2025-07-15 3:45 UTC (permalink / raw)
To: Dmitry Antipov
Cc: Arve Hjønnevåg, Todd Kjos, Martijn Coenen,
Tiffany Y . Yang, linux-kernel, Greg Kroah-Hartman
On Thu, Jun 26, 2025 at 10:30:53AM +0300, Dmitry Antipov wrote:
> In 'binderfs_binder_device_create()', use 'kstrdup()' to copy the
> newly created device's name, thus making the former a bit simpler.
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> drivers/android/binderfs.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 024275dbfdd8..4f827152d18e 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -117,7 +117,6 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
> struct dentry *dentry, *root;
> struct binder_device *device;
> char *name = NULL;
> - size_t name_len;
> struct inode *inode = NULL;
> struct super_block *sb = ref_inode->i_sb;
> struct binderfs_info *info = sb->s_fs_info;
> @@ -161,9 +160,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
> inode->i_gid = info->root_gid;
>
> req->name[BINDERFS_MAX_NAME] = '\0'; /* NUL-terminate */
> - name_len = strlen(req->name);
> - /* Make sure to include terminating NUL byte */
> - name = kmemdup(req->name, name_len + 1, GFP_KERNEL);
> + name = kstrdup(req->name, GFP_KERNEL);
> if (!name)
> goto err;
>
> --
> 2.50.0
>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Thanks Dimitry,
Acked-by: Carlos Llamas <cmllamas@google.com>
^ permalink raw reply [flat|nested] 7+ messages in thread