* [PATCH 3/4] media: rc: Fix ABBA deadlock by making locks more fine grained
2026-08-18 9:36 [PATCH 0/4] Fix locking issues in rc core Sean Young
2026-08-18 9:36 ` [PATCH 1/4] media: rc: Ensure registered is cleared in error path Sean Young
2026-08-18 9:36 ` [PATCH 2/4] media: rc: Ensure that rc_unregister_device() does not free input device Sean Young
@ 2026-08-18 9:36 ` Sean Young
2026-08-18 9:36 ` [PATCH 4/4] media: rc: Add missing locking for keymap Sean Young
3 siblings, 0 replies; 5+ messages in thread
From: Sean Young @ 2026-08-18 9:36 UTC (permalink / raw)
To: linux-media, Sean Young, Mauro Carvalho Chehab, Heiner Kallweit
Cc: Rik van Riel, stable, linux-kernel
The ir_raw_event_unregister() function takes the ir_raw_handler_lock
first and then the rc_dev->lock. Other functions like change_protocol
do this in the reverse order.
This means that an ir decoder module unload and writing to the protocol
sysfs file can cause a deadlock.
ir_raw_handler_lock protects the client list, amongst other things.
Split this out into a separate lock so we can ensure the locking
order is always the same.
Fixes: 93cffffc18f6 ("[media] media: rc: fix decoder module unloading")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
drivers/media/rc/rc-ir-raw.c | 39 ++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 54b323becb1f..75dee4a483d4 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -10,7 +10,8 @@
#include <linux/sched.h>
#include "rc-core-priv.h"
-/* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */
+/* Used to keep track of IR raw clients, protected by ir_raw_client_lock */
+static DEFINE_MUTEX(ir_raw_client_lock);
static LIST_HEAD(ir_raw_client_list);
/* Used to handle IR raw handler extensions */
@@ -273,13 +274,6 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_proto)
return 0;
}
-static void ir_raw_disable_protocols(struct rc_dev *dev, u64 protocols)
-{
- mutex_lock(&dev->lock);
- dev->enabled_protocols &= ~protocols;
- mutex_unlock(&dev->lock);
-}
-
/**
* ir_raw_gen_manchester() - Encode data with Manchester (bi-phase) modulation.
* @ev: Pointer to pointer to next free event. *@ev is incremented for
@@ -615,15 +609,18 @@ int ir_raw_event_register(struct rc_dev *dev)
{
struct task_struct *thread;
+ /* Holding dev->lock could result in a dead-lock */
+ lockdep_assert_not_held(&dev->lock);
+
thread = kthread_run(ir_raw_event_thread, dev->raw, "rc%u", dev->minor);
if (IS_ERR(thread))
return PTR_ERR(thread);
dev->raw->thread = thread;
- mutex_lock(&ir_raw_handler_lock);
+ mutex_lock(&ir_raw_client_lock);
list_add_tail(&dev->raw->list, &ir_raw_client_list);
- mutex_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&ir_raw_client_lock);
return 0;
}
@@ -656,16 +653,19 @@ void ir_raw_event_unregister(struct rc_dev *dev)
kthread_stop(dev->raw->thread);
timer_delete_sync(&dev->raw->edge_handle);
- mutex_lock(&ir_raw_handler_lock);
+ mutex_lock(&ir_raw_client_lock);
list_del(&dev->raw->list);
+
+ mutex_lock(&ir_raw_handler_lock);
list_for_each_entry(handler, &ir_raw_handler_list, list)
if (handler->raw_unregister &&
(handler->protocols & dev->enabled_protocols))
handler->raw_unregister(dev);
lirc_bpf_free(dev);
-
mutex_unlock(&ir_raw_handler_lock);
+
+ mutex_unlock(&ir_raw_client_lock);
}
/*
@@ -688,15 +688,24 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)
struct ir_raw_event_ctrl *raw;
u64 protocols = ir_raw_handler->protocols;
+ mutex_lock(&ir_raw_client_lock);
+
mutex_lock(&ir_raw_handler_lock);
list_del(&ir_raw_handler->list);
+ atomic64_andnot(protocols, &available_protocols);
+ mutex_unlock(&ir_raw_handler_lock);
+
list_for_each_entry(raw, &ir_raw_client_list, list) {
+ mutex_lock(&raw->dev->lock);
+ mutex_lock(&ir_raw_handler_lock);
if (ir_raw_handler->raw_unregister &&
(raw->dev->enabled_protocols & protocols))
ir_raw_handler->raw_unregister(raw->dev);
- ir_raw_disable_protocols(raw->dev, protocols);
+ raw->dev->enabled_protocols &= ~protocols;
+ mutex_unlock(&ir_raw_handler_lock);
+ mutex_unlock(&raw->dev->lock);
}
- atomic64_andnot(protocols, &available_protocols);
- mutex_unlock(&ir_raw_handler_lock);
+
+ mutex_unlock(&ir_raw_client_lock);
}
EXPORT_SYMBOL(ir_raw_handler_unregister);
--
2.55.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 4/4] media: rc: Add missing locking for keymap
2026-08-18 9:36 [PATCH 0/4] Fix locking issues in rc core Sean Young
` (2 preceding siblings ...)
2026-08-18 9:36 ` [PATCH 3/4] media: rc: Fix ABBA deadlock by making locks more fine grained Sean Young
@ 2026-08-18 9:36 ` Sean Young
3 siblings, 0 replies; 5+ messages in thread
From: Sean Young @ 2026-08-18 9:36 UTC (permalink / raw)
To: linux-media, Sean Young, Mauro Carvalho Chehab, Patrice Chotard,
Hans Verkuil
Cc: Rik van Riel, stable, linux-kernel
When the rc_map for an rc_dev gets updated, locking is required but is
missing in places. A concurrent scancode lookup and a call to
rc_{register,unregistered}_device() could result in a use-after-free;
this could happen if IR is decoded during those function calls.
We also fix some ugliness like open-coded krealloc() and removing the
pointless alloc member of rc_map.
Add lockdep assertions where locks are required.
Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
drivers/media/rc/rc-main.c | 122 ++++++++++++++++++++++---------------
include/media/rc-map.h | 2 -
2 files changed, 74 insertions(+), 50 deletions(-)
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 10b924186826..2586e1fa0897 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -17,9 +17,8 @@
#include <linux/module.h>
#include "rc-core-priv.h"
-/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */
-#define IR_TAB_MIN_SIZE 256
-#define IR_TAB_MAX_SIZE 8192
+#define IR_TAB_MIN_SIZE 32
+#define IR_TAB_MAX_SIZE 1024
static const struct {
const char *name;
@@ -105,7 +104,6 @@ static struct rc_map_list *seek_rc_map(const char *name)
struct rc_map *rc_map_get(const char *name)
{
-
struct rc_map_list *map;
map = seek_rc_map(name);
@@ -202,7 +200,7 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
* ir_create_table() - initializes a scancode table
* @dev: the rc_dev device
* @rc_map: the rc_map to initialize
- * @name: name to assign to the table
+ * @map_name: name to assign to the table
* @rc_proto: ir type to assign to the new table
* @size: initial size of the table
*
@@ -212,23 +210,33 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)
* return: zero on success or a negative error code
*/
static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
- const char *name, u64 rc_proto, size_t size)
+ const char *map_name, u64 rc_proto, size_t size)
{
- rc_map->name = kstrdup(name, GFP_KERNEL);
- if (!rc_map->name)
+ struct rc_map_table *scan;
+ unsigned int alloc;
+ char *name;
+
+ name = kstrdup(map_name, GFP_KERNEL);
+ if (!name)
return -ENOMEM;
- rc_map->rc_proto = rc_proto;
- rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
- rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
- rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);
- if (!rc_map->scan) {
- kfree(rc_map->name);
- rc_map->name = NULL;
+
+ alloc = roundup_pow_of_two(size);
+ scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);
+ if (!scan) {
+ kfree(name);
return -ENOMEM;
}
- dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%u bytes)\n",
- rc_map->size, rc_map->alloc);
+ scoped_guard(spinlock_irqsave, &dev->rc_map.lock) {
+ rc_map->name = name;
+ rc_map->scan = scan;
+ rc_map->rc_proto = rc_proto;
+ rc_map->len = 0;
+ rc_map->size = alloc;
+ }
+
+ dev_dbg(&dev->dev, "Allocated space for %u keycode entries (%zu bytes)\n",
+ rc_map->size, rc_map->size * sizeof(struct rc_map_table));
return 0;
}
@@ -236,16 +244,26 @@ static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,
* ir_free_table() - frees memory allocated by a scancode table
* @rc_map: the table whose mappings need to be freed
*
- * This routine will free memory alloctaed for key mappings used by given
+ * This routine will free memory allocated for key mappings used by given
* scancode table.
*/
static void ir_free_table(struct rc_map *rc_map)
{
- rc_map->size = 0;
- kfree(rc_map->name);
- rc_map->name = NULL;
- kfree(rc_map->scan);
- rc_map->scan = NULL;
+ struct rc_map_table *scan;
+ const char *name;
+
+ scoped_guard(spinlock_irqsave, &rc_map->lock) {
+ name = rc_map->name;
+ scan = rc_map->scan;
+
+ rc_map->size = 0;
+ rc_map->len = 0;
+ rc_map->name = NULL;
+ rc_map->scan = NULL;
+ }
+
+ kfree(name);
+ kfree(scan);
}
/**
@@ -262,38 +280,38 @@ static void ir_free_table(struct rc_map *rc_map)
static int ir_resize_table(struct rc_dev *dev, struct rc_map *rc_map,
gfp_t gfp_flags)
{
- unsigned int oldalloc = rc_map->alloc;
- unsigned int newalloc = oldalloc;
- struct rc_map_table *oldscan = rc_map->scan;
+ unsigned int newsize = rc_map->size;
struct rc_map_table *newscan;
+ lockdep_assert_held(&rc_map->lock);
+
if (rc_map->size == rc_map->len) {
/* All entries in use -> grow keytable */
- if (rc_map->alloc >= IR_TAB_MAX_SIZE)
+ newsize *= 2;
+
+ if (newsize >= IR_TAB_MAX_SIZE)
return -ENOMEM;
- newalloc *= 2;
- dev_dbg(&dev->dev, "Growing table to %u bytes\n", newalloc);
+ dev_dbg(&dev->dev, "Growing table to %u entries\n", newsize);
}
- if ((rc_map->len * 3 < rc_map->size) && (oldalloc > IR_TAB_MIN_SIZE)) {
+ if (rc_map->len * 3 < rc_map->size && rc_map->size > IR_TAB_MIN_SIZE) {
/* Less than 1/3 of entries in use -> shrink keytable */
- newalloc /= 2;
- dev_dbg(&dev->dev, "Shrinking table to %u bytes\n", newalloc);
+ newsize /= 2;
+ dev_dbg(&dev->dev, "Shrinking table to %u entries\n", newsize);
}
- if (newalloc == oldalloc)
+ if (newsize == rc_map->size)
return 0;
- newscan = kmalloc(newalloc, gfp_flags);
+ newscan = krealloc_array(rc_map->scan, newsize,
+ sizeof(struct rc_map_table), gfp_flags);
if (!newscan)
return -ENOMEM;
- memcpy(newscan, rc_map->scan, rc_map->len * sizeof(struct rc_map_table));
rc_map->scan = newscan;
- rc_map->alloc = newalloc;
- rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
- kfree(oldscan);
+ rc_map->size = newsize;
+
return 0;
}
@@ -318,6 +336,8 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
int old_keycode = rc_map->scan[index].keycode;
int i;
+ lockdep_assert_held(&rc_map->lock);
+
/* Did the user wish to remove the mapping? */
if (new_keycode == KEY_RESERVED || new_keycode == KEY_UNKNOWN) {
dev_dbg(&dev->dev, "#%d: Deleting scan 0x%04llx\n",
@@ -373,6 +393,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
{
unsigned int i;
+ lockdep_assert_held(&rc_map->lock);
+
/*
* Unfortunately, some hardware-based IR decoders don't provide
* all bits for the complete IR code. In general, they provide only
@@ -397,7 +419,7 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
/* No previous mapping found, we might need to grow the table */
if (rc_map->size == rc_map->len) {
if (!resize || ir_resize_table(dev, rc_map, GFP_ATOMIC))
- return -1U;
+ return UINT_MAX;
}
/* i is the proper index to insert our new keycode */
@@ -479,16 +501,18 @@ static int ir_setkeytable(struct rc_dev *dev, const struct rc_map *from)
if (rc)
return rc;
- for (i = 0; i < from->size; i++) {
- index = ir_establish_scancode(dev, rc_map,
- from->scan[i].scancode, false);
- if (index >= rc_map->len) {
- rc = -ENOMEM;
- break;
- }
+ scoped_guard(spinlock_irqsave, &dev->rc_map.lock) {
+ for (i = 0; i < from->size; i++) {
+ index = ir_establish_scancode(dev, rc_map,
+ from->scan[i].scancode, false);
+ if (index >= rc_map->len) {
+ rc = -ENOMEM;
+ break;
+ }
- ir_update_mapping(dev, rc_map, index,
- from->scan[i].keycode);
+ ir_update_mapping(dev, rc_map, index,
+ from->scan[i].keycode);
+ }
}
if (rc)
@@ -524,6 +548,8 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
{
struct rc_map_table *res;
+ lockdep_assert_held(&rc_map->lock);
+
res = bsearch(&scancode, rc_map->scan, rc_map->len,
sizeof(struct rc_map_table), rc_map_cmp);
if (!res)
diff --git a/include/media/rc-map.h b/include/media/rc-map.h
index d95ed3e96de2..f167c37179c8 100644
--- a/include/media/rc-map.h
+++ b/include/media/rc-map.h
@@ -148,7 +148,6 @@ struct rc_map_table {
* @scan: pointer to struct &rc_map_table
* @size: Max number of entries
* @len: Number of entries that are in use
- * @alloc: size of \*scan, in bytes
* @rc_proto: type of the remote controller protocol, as defined at
* enum &rc_proto
* @name: name of the key map table
@@ -158,7 +157,6 @@ struct rc_map {
struct rc_map_table *scan;
unsigned int size;
unsigned int len;
- unsigned int alloc;
enum rc_proto rc_proto;
const char *name;
spinlock_t lock;
--
2.55.0
^ permalink raw reply related [flat|nested] 5+ messages in thread