* Re: [PATCH v3] HID: multitouch: use GFP_ATOMIC for report/application/usage allocations
[not found] <20260910164229.CC7E51F0089E@smtp.kernel.org>
@ 2026-09-10 16:54 ` Nguyen Ngoc Thang
2026-09-10 16:55 ` [PATCH v2] " Nguyen Ngoc Thang
1 sibling, 0 replies; 3+ messages in thread
From: Nguyen Ngoc Thang @ 2026-09-10 16:54 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, syzbot+093e05755c2d99caab91,
Dmitry Torokhov, sashiko-bot, Nguyen Ngoc Thang
mt_allocate_report_data() and mt_allocate_application() devm_kzalloc()
with GFP_KERNEL, but they are reachable from mt_event()/mt_report()
via hid_report_raw_event(), which can run in (soft)irq context off a
USB URB completion (e.g. dummy_hcd's hrtimer softirq). GFP_KERNEL
there may call into fs_reclaim, which lockdep flags as an
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} use of fs_reclaim,
alongside a "sleeping function called from invalid context" splat.
Switch both to GFP_ATOMIC since neither is on a path that needs to
sleep. mt_allocate_usage() is left at GFP_KERNEL: it is only reached
from mt_touch_input_mapping(), which runs at probe time (hid_hw_start()),
never from mt_event(), so it does not need the downgrade.
While auditing that same softirq path, td->applications turned out to
be walked and mutated without any lock: mt_allocate_application() can
list_add_tail() to it from mt_event()/mt_report() in (soft)irq
context, while mt_release_contacts() (timer callback) and
mt_set_quirks() (sysfs write) each list_for_each_entry() over it from
their own, independent contexts. On SMP those can run concurrently
and corrupt the list. Add application_list_lock (spin_lock_irqsave,
since one user is a timer/irq context) around every access to
td->applications, and hold it across mt_find_application()'s whole
search-then-allocate sequence so two callers can never both decide to
create the same application.
Reported-by: syzbot+093e05755c2d99caab91@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=093e05755c2d99caab91
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
v3:
- Add application_list_lock to serialize td->applications against the
concurrent softirq/timer/sysfs accessors flagged by sashiko-bot
(pre-existing race, not introduced by this patch, but fixed here
since it's on the same softirq-safety path).
v2:
- Keep mt_allocate_usage() at GFP_KERNEL. It is only reached from
mt_touch_input_mapping(), at probe time, never from mt_event(), so
the GFP_ATOMIC downgrade there was unnecessary.
(feedback from sashiko-bot)
---
drivers/hid/hid-multitouch.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2c41bacab1ca..dd17bf4abfae 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -37,6 +37,7 @@
#include <linux/hid.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/input/mt.h>
#include <linux/jiffies.h>
#include <linux/string.h>
@@ -186,6 +187,11 @@ struct mt_device {
bool serial_maybe; /* need to check for serial protocol */
struct list_head applications;
+ spinlock_t application_list_lock; /* protects applications, incl.
+ * concurrent add from softirq
+ * vs. list_for_each_entry from
+ * timer/sysfs context
+ */
struct list_head reports;
};
@@ -477,6 +483,7 @@ static ssize_t mt_set_quirks(struct device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct mt_device *td = hid_get_drvdata(hdev);
struct mt_application *application;
+ unsigned long flags;
unsigned long val;
@@ -485,11 +492,13 @@ static ssize_t mt_set_quirks(struct device *dev,
td->mtclass.quirks = val;
+ spin_lock_irqsave(&td->application_list_lock, flags);
list_for_each_entry(application, &td->applications, list) {
application->quirks = val;
if (!application->have_contact_count)
application->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
}
+ spin_unlock_irqrestore(&td->application_list_lock, flags);
return count;
}
@@ -604,6 +613,7 @@ static struct mt_usages *mt_allocate_usage(struct hid_device *hdev,
{
struct mt_usages *usage;
+ /* only called from mt_touch_input_mapping(), at probe time */
usage = devm_kzalloc(&hdev->dev, sizeof(*usage), GFP_KERNEL);
if (!usage)
return NULL;
@@ -627,14 +637,16 @@ static struct mt_usages *mt_allocate_usage(struct hid_device *hdev,
return usage;
}
+/* called with td->application_list_lock held */
static struct mt_application *mt_allocate_application(struct mt_device *td,
struct hid_report *report)
{
unsigned int application = report->application;
struct mt_application *mt_application;
+ /* may run from hid_report_raw_event() in (soft)irq context */
mt_application = devm_kzalloc(&td->hdev->dev, sizeof(*mt_application),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!mt_application)
return NULL;
@@ -667,6 +679,15 @@ static struct mt_application *mt_find_application(struct mt_device *td,
{
unsigned int application = report->application;
struct mt_application *tmp, *mt_application = NULL;
+ unsigned long flags;
+
+ /*
+ * Hold the lock across the search and the fallback allocation so
+ * a concurrent mt_release_contacts()/mt_set_quirks() never walks
+ * the list mid-insert, and two callers can never both decide to
+ * allocate the same application.
+ */
+ spin_lock_irqsave(&td->application_list_lock, flags);
list_for_each_entry(tmp, &td->applications, list) {
if (application == tmp->application) {
@@ -681,6 +702,8 @@ static struct mt_application *mt_find_application(struct mt_device *td,
if (!mt_application)
mt_application = mt_allocate_application(td, report);
+ spin_unlock_irqrestore(&td->application_list_lock, flags);
+
return mt_application;
}
@@ -692,7 +715,8 @@ static struct mt_report_data *mt_allocate_report_data(struct mt_device *td,
struct hid_field *field;
int r, n;
- rdata = devm_kzalloc(&td->hdev->dev, sizeof(*rdata), GFP_KERNEL);
+ /* may run from hid_report_raw_event() in (soft)irq context */
+ rdata = devm_kzalloc(&td->hdev->dev, sizeof(*rdata), GFP_ATOMIC);
if (!rdata)
return NULL;
@@ -2060,6 +2084,7 @@ static void mt_release_contacts(struct hid_device *hid)
struct hid_input *hidinput;
struct mt_application *application;
struct mt_device *td = hid_get_drvdata(hid);
+ unsigned long flags;
list_for_each_entry(hidinput, &hid->inputs, list) {
struct input_dev *input_dev = hidinput->input;
@@ -2077,9 +2102,11 @@ static void mt_release_contacts(struct hid_device *hid)
}
}
+ spin_lock_irqsave(&td->application_list_lock, flags);
list_for_each_entry(application, &td->applications, list) {
application->num_received = 0;
}
+ spin_unlock_irqrestore(&td->application_list_lock, flags);
}
static void mt_expired_timeout(struct timer_list *t)
@@ -2127,6 +2154,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
hid_set_drvdata(hdev, td);
INIT_LIST_HEAD(&td->applications);
+ spin_lock_init(&td->application_list_lock);
INIT_LIST_HEAD(&td->reports);
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] HID: multitouch: use GFP_ATOMIC for report/application/usage allocations
[not found] <20260910164229.CC7E51F0089E@smtp.kernel.org>
2026-09-10 16:54 ` [PATCH v3] HID: multitouch: use GFP_ATOMIC for report/application/usage allocations Nguyen Ngoc Thang
@ 2026-09-10 16:55 ` Nguyen Ngoc Thang
1 sibling, 0 replies; 3+ messages in thread
From: Nguyen Ngoc Thang @ 2026-09-10 16:55 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, syzbot+093e05755c2d99caab91,
Dmitry Torokhov, sashiko-bot, Nguyen Ngoc Thang
mt_allocate_report_data() and mt_allocate_application() devm_kzalloc()
with GFP_KERNEL, but they are reachable from mt_event()/mt_report()
via hid_report_raw_event(), which can run in (soft)irq context off a
USB URB completion (e.g. dummy_hcd's hrtimer softirq). GFP_KERNEL
there may call into fs_reclaim, which lockdep flags as an
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} use of fs_reclaim,
alongside a "sleeping function called from invalid context" splat.
Switch both to GFP_ATOMIC since neither is on a path that needs to
sleep. mt_allocate_usage() is left at GFP_KERNEL: it is only reached
from mt_touch_input_mapping(), which runs at probe time (hid_hw_start()),
never from mt_event(), so it does not need the downgrade.
While auditing that same softirq path, td->applications turned out to
be walked and mutated without any lock: mt_allocate_application() can
list_add_tail() to it from mt_event()/mt_report() in (soft)irq
context, while mt_release_contacts() (timer callback) and
mt_set_quirks() (sysfs write) each list_for_each_entry() over it from
their own, independent contexts. On SMP those can run concurrently
and corrupt the list. Add application_list_lock (spin_lock_irqsave,
since one user is a timer/irq context) around every access to
td->applications, and hold it across mt_find_application()'s whole
search-then-allocate sequence so two callers can never both decide to
create the same application.
Reported-by: syzbot+093e05755c2d99caab91@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=093e05755c2d99caab91
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
v3:
- Add application_list_lock to serialize td->applications against the
concurrent softirq/timer/sysfs accessors flagged by sashiko-bot
(pre-existing race, not introduced by this patch, but fixed here
since it's on the same softirq-safety path).
v2:
- Keep mt_allocate_usage() at GFP_KERNEL. It is only reached from
mt_touch_input_mapping(), at probe time, never from mt_event(), so
the GFP_ATOMIC downgrade there was unnecessary.
(feedback from sashiko-bot)
---
drivers/hid/hid-multitouch.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2c41bacab1ca..dd17bf4abfae 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -37,6 +37,7 @@
#include <linux/hid.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
#include <linux/input/mt.h>
#include <linux/jiffies.h>
#include <linux/string.h>
@@ -186,6 +187,11 @@ struct mt_device {
bool serial_maybe; /* need to check for serial protocol */
struct list_head applications;
+ spinlock_t application_list_lock; /* protects applications, incl.
+ * concurrent add from softirq
+ * vs. list_for_each_entry from
+ * timer/sysfs context
+ */
struct list_head reports;
};
@@ -477,6 +483,7 @@ static ssize_t mt_set_quirks(struct device *dev,
struct hid_device *hdev = to_hid_device(dev);
struct mt_device *td = hid_get_drvdata(hdev);
struct mt_application *application;
+ unsigned long flags;
unsigned long val;
@@ -485,11 +492,13 @@ static ssize_t mt_set_quirks(struct device *dev,
td->mtclass.quirks = val;
+ spin_lock_irqsave(&td->application_list_lock, flags);
list_for_each_entry(application, &td->applications, list) {
application->quirks = val;
if (!application->have_contact_count)
application->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
}
+ spin_unlock_irqrestore(&td->application_list_lock, flags);
return count;
}
@@ -604,6 +613,7 @@ static struct mt_usages *mt_allocate_usage(struct hid_device *hdev,
{
struct mt_usages *usage;
+ /* only called from mt_touch_input_mapping(), at probe time */
usage = devm_kzalloc(&hdev->dev, sizeof(*usage), GFP_KERNEL);
if (!usage)
return NULL;
@@ -627,14 +637,16 @@ static struct mt_usages *mt_allocate_usage(struct hid_device *hdev,
return usage;
}
+/* called with td->application_list_lock held */
static struct mt_application *mt_allocate_application(struct mt_device *td,
struct hid_report *report)
{
unsigned int application = report->application;
struct mt_application *mt_application;
+ /* may run from hid_report_raw_event() in (soft)irq context */
mt_application = devm_kzalloc(&td->hdev->dev, sizeof(*mt_application),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!mt_application)
return NULL;
@@ -667,6 +679,15 @@ static struct mt_application *mt_find_application(struct mt_device *td,
{
unsigned int application = report->application;
struct mt_application *tmp, *mt_application = NULL;
+ unsigned long flags;
+
+ /*
+ * Hold the lock across the search and the fallback allocation so
+ * a concurrent mt_release_contacts()/mt_set_quirks() never walks
+ * the list mid-insert, and two callers can never both decide to
+ * allocate the same application.
+ */
+ spin_lock_irqsave(&td->application_list_lock, flags);
list_for_each_entry(tmp, &td->applications, list) {
if (application == tmp->application) {
@@ -681,6 +702,8 @@ static struct mt_application *mt_find_application(struct mt_device *td,
if (!mt_application)
mt_application = mt_allocate_application(td, report);
+ spin_unlock_irqrestore(&td->application_list_lock, flags);
+
return mt_application;
}
@@ -692,7 +715,8 @@ static struct mt_report_data *mt_allocate_report_data(struct mt_device *td,
struct hid_field *field;
int r, n;
- rdata = devm_kzalloc(&td->hdev->dev, sizeof(*rdata), GFP_KERNEL);
+ /* may run from hid_report_raw_event() in (soft)irq context */
+ rdata = devm_kzalloc(&td->hdev->dev, sizeof(*rdata), GFP_ATOMIC);
if (!rdata)
return NULL;
@@ -2060,6 +2084,7 @@ static void mt_release_contacts(struct hid_device *hid)
struct hid_input *hidinput;
struct mt_application *application;
struct mt_device *td = hid_get_drvdata(hid);
+ unsigned long flags;
list_for_each_entry(hidinput, &hid->inputs, list) {
struct input_dev *input_dev = hidinput->input;
@@ -2077,9 +2102,11 @@ static void mt_release_contacts(struct hid_device *hid)
}
}
+ spin_lock_irqsave(&td->application_list_lock, flags);
list_for_each_entry(application, &td->applications, list) {
application->num_received = 0;
}
+ spin_unlock_irqrestore(&td->application_list_lock, flags);
}
static void mt_expired_timeout(struct timer_list *t)
@@ -2127,6 +2154,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
hid_set_drvdata(hdev, td);
INIT_LIST_HEAD(&td->applications);
+ spin_lock_init(&td->application_list_lock);
INIT_LIST_HEAD(&td->reports);
if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread