This series contains some tidying up of gpiolib-cdev following the
recent adoption of guard().
The first patch is a fix to protect gpio_ioctl() from having the
gpio chip removed while the ioctl is in progress.
The next couple of patches are minor fixes inspired by recent
submissions and reviews for gpiolib.c.
Patch 2 adds a missing include.
Patch 3 switches allocation of struct linereq from kzalloc() to
kvzalloc() as it can be larger than one page - even more so after the
recent relocation of debounce_period_us.
The final two patches replace wrapper functions with guards.
Patch 4 tidies up the functions that use a guard on the linereq
config_mutex.
Patch 5 tidies up the functions that use a guard on the gpio_device.
Changes v1 -> v2:
- add patch 1 to protect gpio_ioctl() from chip removal
- improve commit comment (patch 3)
- use guard(rwsem_read) rather than rolling our own (patch 5)
Cheers,
Kent.
Kent Gibson (5):
gpiolib: cdev: add gpio_device locking wrapper around gpio_ioctl()
gpiolib: cdev: include overflow.h
gpiolib: cdev: allocate linereq using kvzalloc()
gpiolib: cdev: replace locking wrappers for config_mutex with guards
gpiolib: cdev: replace locking wrappers for gpio_device with guards
drivers/gpio/gpiolib-cdev.c | 257 ++++++++++--------------------------
1 file changed, 70 insertions(+), 187 deletions(-)
--
2.39.2
While the GPIO cdev gpio_ioctl() call is in progress, the kernel can
call gpiochip_remove() which will set gdev->chip to NULL, after which
any subsequent access will cause a crash.
gpio_ioctl() was overlooked by the previous fix to protect syscalls
(bdbbae241a04), so add protection for that.
Fixes: bdbbae241a04 ("gpiolib: protect the GPIO device against being dropped while in use by user-space")
Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 744734405912..9155c54acc1e 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2598,10 +2598,7 @@ static int lineinfo_unwatch(struct gpio_chardev_data *cdev, void __user *ip)
return 0;
}
-/*
- * gpio_ioctl() - ioctl handler for the GPIO chardev
- */
-static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+static long gpio_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned long arg)
{
struct gpio_chardev_data *cdev = file->private_data;
struct gpio_device *gdev = cdev->gdev;
@@ -2638,6 +2635,17 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
}
}
+/*
+ * gpio_ioctl() - ioctl handler for the GPIO chardev
+ */
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct gpio_chardev_data *cdev = file->private_data;
+
+ return call_ioctl_locked(file, cmd, arg, cdev->gdev,
+ gpio_ioctl_unlocked);
+}
+
#ifdef CONFIG_COMPAT
static long gpio_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
--
2.39.2
struct_size() is used to calculate struct linereq size, so explicitly
include overflow.h.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 9155c54acc1e..942fe115b726 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -20,6 +20,7 @@
#include <linux/kfifo.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/overflow.h>
#include <linux/pinctrl/consumer.h>
#include <linux/poll.h>
#include <linux/rbtree.h>
--
2.39.2
The size of struct linereq may exceed a page, so allocate space for
it using kvzalloc() instead of kzalloc() to handle the case where
memory is heavily fragmented and kzalloc() cannot find a sufficient
contiguous region.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 942fe115b726..5424c878627e 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1723,7 +1723,7 @@ static void linereq_free(struct linereq *lr)
kfifo_free(&lr->events);
kfree(lr->label);
gpio_device_put(lr->gdev);
- kfree(lr);
+ kvfree(lr);
}
static int linereq_release(struct inode *inode, struct file *file)
@@ -1788,7 +1788,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
if (ret)
return ret;
- lr = kzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
+ lr = kvzalloc(struct_size(lr, lines, ulr.num_lines), GFP_KERNEL);
if (!lr)
return -ENOMEM;
lr->num_lines = ulr.num_lines;
--
2.39.2
After the adoption of guard(), the locking wrappers that hold the
config_mutex for linereq_set_values() and linereq_set_config() no
longer add value, so combine them into the functions they wrap.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 63 ++++++++++++++-----------------------
1 file changed, 24 insertions(+), 39 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5424c878627e..9ff2b447cc20 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1454,14 +1454,19 @@ static long linereq_get_values(struct linereq *lr, void __user *ip)
return 0;
}
-static long linereq_set_values_unlocked(struct linereq *lr,
- struct gpio_v2_line_values *lv)
+static long linereq_set_values(struct linereq *lr, void __user *ip)
{
DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
+ struct gpio_v2_line_values lv;
struct gpio_desc **descs;
unsigned int i, didx, num_set;
int ret;
+ if (copy_from_user(&lv, ip, sizeof(lv)))
+ return -EFAULT;
+
+ guard(mutex)(&lr->config_mutex);
+
/*
* gpiod_set_array_value_complex() requires compacted desc and val
* arrays, rather than the sparse ones in lv.
@@ -1472,12 +1477,12 @@ static long linereq_set_values_unlocked(struct linereq *lr,
bitmap_zero(vals, GPIO_V2_LINES_MAX);
/* scan requested lines to determine the subset to be set */
for (num_set = 0, i = 0; i < lr->num_lines; i++) {
- if (lv->mask & BIT_ULL(i)) {
+ if (lv.mask & BIT_ULL(i)) {
/* setting inputs is not allowed */
if (!test_bit(FLAG_IS_OUT, &lr->lines[i].desc->flags))
return -EPERM;
/* add to compacted values */
- if (lv->bits & BIT_ULL(i))
+ if (lv.bits & BIT_ULL(i))
__set_bit(num_set, vals);
num_set++;
/* capture desc for the num_set == 1 case */
@@ -1493,7 +1498,7 @@ static long linereq_set_values_unlocked(struct linereq *lr,
if (!descs)
return -ENOMEM;
for (didx = 0, i = 0; i < lr->num_lines; i++) {
- if (lv->mask & BIT_ULL(i)) {
+ if (lv.mask & BIT_ULL(i)) {
descs[didx] = lr->lines[i].desc;
didx++;
}
@@ -1507,31 +1512,28 @@ static long linereq_set_values_unlocked(struct linereq *lr,
return ret;
}
-static long linereq_set_values(struct linereq *lr, void __user *ip)
-{
- struct gpio_v2_line_values lv;
-
- if (copy_from_user(&lv, ip, sizeof(lv)))
- return -EFAULT;
-
- guard(mutex)(&lr->config_mutex);
-
- return linereq_set_values_unlocked(lr, &lv);
-}
-
-static long linereq_set_config_unlocked(struct linereq *lr,
- struct gpio_v2_line_config *lc)
+static long linereq_set_config(struct linereq *lr, void __user *ip)
{
+ struct gpio_v2_line_config lc;
struct gpio_desc *desc;
struct line *line;
unsigned int i;
u64 flags, edflags;
int ret;
+ if (copy_from_user(&lc, ip, sizeof(lc)))
+ return -EFAULT;
+
+ ret = gpio_v2_line_config_validate(&lc, lr->num_lines);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&lr->config_mutex);
+
for (i = 0; i < lr->num_lines; i++) {
line = &lr->lines[i];
desc = lr->lines[i].desc;
- flags = gpio_v2_line_config_flags(lc, i);
+ flags = gpio_v2_line_config_flags(&lc, i);
gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags);
edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS;
/*
@@ -1539,7 +1541,7 @@ static long linereq_set_config_unlocked(struct linereq *lr,
* or output, else the line will be treated "as is".
*/
if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
- int val = gpio_v2_line_config_output_value(lc, i);
+ int val = gpio_v2_line_config_output_value(&lc, i);
edge_detector_stop(line);
ret = gpiod_direction_output(desc, val);
@@ -1550,7 +1552,7 @@ static long linereq_set_config_unlocked(struct linereq *lr,
if (ret)
return ret;
- ret = edge_detector_update(line, lc, i, edflags);
+ ret = edge_detector_update(line, &lc, i, edflags);
if (ret)
return ret;
}
@@ -1562,23 +1564,6 @@ static long linereq_set_config_unlocked(struct linereq *lr,
return 0;
}
-static long linereq_set_config(struct linereq *lr, void __user *ip)
-{
- struct gpio_v2_line_config lc;
- int ret;
-
- if (copy_from_user(&lc, ip, sizeof(lc)))
- return -EFAULT;
-
- ret = gpio_v2_line_config_validate(&lc, lr->num_lines);
- if (ret)
- return ret;
-
- guard(mutex)(&lr->config_mutex);
-
- return linereq_set_config_unlocked(lr, &lc);
-}
-
static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
unsigned long arg)
{
--
2.39.2
Replace the wrapping functions that inhibit removal of the gpio chip
with equivalent guards.
Signed-off-by: Kent Gibson <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 205 +++++++++---------------------------
1 file changed, 47 insertions(+), 158 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 9ff2b447cc20..2a88736629ef 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -24,6 +24,7 @@
#include <linux/pinctrl/consumer.h>
#include <linux/poll.h>
#include <linux/rbtree.h>
+#include <linux/rwsem.h>
#include <linux/seq_file.h>
#include <linux/spinlock.h>
#include <linux/timekeeping.h>
@@ -65,45 +66,6 @@ typedef long (*ioctl_fn)(struct file *, unsigned int, unsigned long);
typedef ssize_t (*read_fn)(struct file *, char __user *,
size_t count, loff_t *);
-static __poll_t call_poll_locked(struct file *file,
- struct poll_table_struct *wait,
- struct gpio_device *gdev, poll_fn func)
-{
- __poll_t ret;
-
- down_read(&gdev->sem);
- ret = func(file, wait);
- up_read(&gdev->sem);
-
- return ret;
-}
-
-static long call_ioctl_locked(struct file *file, unsigned int cmd,
- unsigned long arg, struct gpio_device *gdev,
- ioctl_fn func)
-{
- long ret;
-
- down_read(&gdev->sem);
- ret = func(file, cmd, arg);
- up_read(&gdev->sem);
-
- return ret;
-}
-
-static ssize_t call_read_locked(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps,
- struct gpio_device *gdev, read_fn func)
-{
- ssize_t ret;
-
- down_read(&gdev->sem);
- ret = func(file, buf, count, f_ps);
- up_read(&gdev->sem);
-
- return ret;
-}
-
/*
* GPIO line handle management
*/
@@ -238,8 +200,8 @@ static long linehandle_set_config(struct linehandle_state *lh,
return 0;
}
-static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
- unsigned long arg)
+static long linehandle_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
struct linehandle_state *lh = file->private_data;
void __user *ip = (void __user *)arg;
@@ -248,6 +210,8 @@ static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
unsigned int i;
int ret;
+ guard(rwsem_read)(&lh->gdev->sem);
+
if (!lh->gdev->chip)
return -ENODEV;
@@ -297,15 +261,6 @@ static long linehandle_ioctl_unlocked(struct file *file, unsigned int cmd,
}
}
-static long linehandle_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- struct linehandle_state *lh = file->private_data;
-
- return call_ioctl_locked(file, cmd, arg, lh->gdev,
- linehandle_ioctl_unlocked);
-}
-
#ifdef CONFIG_COMPAT
static long linehandle_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -1564,12 +1519,14 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
return 0;
}
-static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
- unsigned long arg)
+static long linereq_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
struct linereq *lr = file->private_data;
void __user *ip = (void __user *)arg;
+ guard(rwsem_read)(&lr->gdev->sem);
+
if (!lr->gdev->chip)
return -ENODEV;
@@ -1585,15 +1542,6 @@ static long linereq_ioctl_unlocked(struct file *file, unsigned int cmd,
}
}
-static long linereq_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- struct linereq *lr = file->private_data;
-
- return call_ioctl_locked(file, cmd, arg, lr->gdev,
- linereq_ioctl_unlocked);
-}
-
#ifdef CONFIG_COMPAT
static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -1602,12 +1550,14 @@ static long linereq_ioctl_compat(struct file *file, unsigned int cmd,
}
#endif
-static __poll_t linereq_poll_unlocked(struct file *file,
- struct poll_table_struct *wait)
+static __poll_t linereq_poll(struct file *file,
+ struct poll_table_struct *wait)
{
struct linereq *lr = file->private_data;
__poll_t events = 0;
+ guard(rwsem_read)(&lr->gdev->sem);
+
if (!lr->gdev->chip)
return EPOLLHUP | EPOLLERR;
@@ -1620,22 +1570,16 @@ static __poll_t linereq_poll_unlocked(struct file *file,
return events;
}
-static __poll_t linereq_poll(struct file *file,
- struct poll_table_struct *wait)
-{
- struct linereq *lr = file->private_data;
-
- return call_poll_locked(file, wait, lr->gdev, linereq_poll_unlocked);
-}
-
-static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
+static ssize_t linereq_read(struct file *file, char __user *buf,
+ size_t count, loff_t *f_ps)
{
struct linereq *lr = file->private_data;
struct gpio_v2_line_event le;
ssize_t bytes_read = 0;
int ret;
+ guard(rwsem_read)(&lr->gdev->sem);
+
if (!lr->gdev->chip)
return -ENODEV;
@@ -1677,15 +1621,6 @@ static ssize_t linereq_read_unlocked(struct file *file, char __user *buf,
return bytes_read;
}
-static ssize_t linereq_read(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
-{
- struct linereq *lr = file->private_data;
-
- return call_read_locked(file, buf, count, f_ps, lr->gdev,
- linereq_read_unlocked);
-}
-
static void linereq_free(struct linereq *lr)
{
struct line *line;
@@ -1938,12 +1873,14 @@ struct lineevent_state {
(GPIOEVENT_REQUEST_RISING_EDGE | \
GPIOEVENT_REQUEST_FALLING_EDGE)
-static __poll_t lineevent_poll_unlocked(struct file *file,
- struct poll_table_struct *wait)
+static __poll_t lineevent_poll(struct file *file,
+ struct poll_table_struct *wait)
{
struct lineevent_state *le = file->private_data;
__poll_t events = 0;
+ guard(rwsem_read)(&le->gdev->sem);
+
if (!le->gdev->chip)
return EPOLLHUP | EPOLLERR;
@@ -1955,14 +1892,6 @@ static __poll_t lineevent_poll_unlocked(struct file *file,
return events;
}
-static __poll_t lineevent_poll(struct file *file,
- struct poll_table_struct *wait)
-{
- struct lineevent_state *le = file->private_data;
-
- return call_poll_locked(file, wait, le->gdev, lineevent_poll_unlocked);
-}
-
static int lineevent_unregistered_notify(struct notifier_block *nb,
unsigned long action, void *data)
{
@@ -1979,8 +1908,8 @@ struct compat_gpioeevent_data {
u32 id;
};
-static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
+static ssize_t lineevent_read(struct file *file, char __user *buf,
+ size_t count, loff_t *f_ps)
{
struct lineevent_state *le = file->private_data;
struct gpioevent_data ge;
@@ -1988,6 +1917,8 @@ static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
ssize_t ge_size;
int ret;
+ guard(rwsem_read)(&le->gdev->sem);
+
if (!le->gdev->chip)
return -ENODEV;
@@ -2042,15 +1973,6 @@ static ssize_t lineevent_read_unlocked(struct file *file, char __user *buf,
return bytes_read;
}
-static ssize_t lineevent_read(struct file *file, char __user *buf,
- size_t count, loff_t *f_ps)
-{
- struct lineevent_state *le = file->private_data;
-
- return call_read_locked(file, buf, count, f_ps, le->gdev,
- lineevent_read_unlocked);
-}
-
static void lineevent_free(struct lineevent_state *le)
{
if (le->device_unregistered_nb.notifier_call)
@@ -2071,13 +1993,15 @@ static int lineevent_release(struct inode *inode, struct file *file)
return 0;
}
-static long lineevent_ioctl_unlocked(struct file *file, unsigned int cmd,
- unsigned long arg)
+static long lineevent_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
{
struct lineevent_state *le = file->private_data;
void __user *ip = (void __user *)arg;
struct gpiohandle_data ghd;
+ guard(rwsem_read)(&le->gdev->sem);
+
if (!le->gdev->chip)
return -ENODEV;
@@ -2103,15 +2027,6 @@ static long lineevent_ioctl_unlocked(struct file *file, unsigned int cmd,
return -EINVAL;
}
-static long lineevent_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg)
-{
- struct lineevent_state *le = file->private_data;
-
- return call_ioctl_locked(file, cmd, arg, le->gdev,
- lineevent_ioctl_unlocked);
-}
-
#ifdef CONFIG_COMPAT
static long lineevent_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -2584,12 +2499,17 @@ static int lineinfo_unwatch(struct gpio_chardev_data *cdev, void __user *ip)
return 0;
}
-static long gpio_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned long arg)
+/*
+ * gpio_ioctl() - ioctl handler for the GPIO chardev
+ */
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
struct gpio_chardev_data *cdev = file->private_data;
struct gpio_device *gdev = cdev->gdev;
void __user *ip = (void __user *)arg;
+ guard(rwsem_read)(&gdev->sem);
+
/* We fail any subsequent ioctl():s when the chip is gone */
if (!gdev->chip)
return -ENODEV;
@@ -2621,17 +2541,6 @@ static long gpio_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned lo
}
}
-/*
- * gpio_ioctl() - ioctl handler for the GPIO chardev
- */
-static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
- struct gpio_chardev_data *cdev = file->private_data;
-
- return call_ioctl_locked(file, cmd, arg, cdev->gdev,
- gpio_ioctl_unlocked);
-}
-
#ifdef CONFIG_COMPAT
static long gpio_ioctl_compat(struct file *file, unsigned int cmd,
unsigned long arg)
@@ -2679,12 +2588,14 @@ static int gpio_device_unregistered_notify(struct notifier_block *nb,
return NOTIFY_OK;
}
-static __poll_t lineinfo_watch_poll_unlocked(struct file *file,
- struct poll_table_struct *pollt)
+static __poll_t lineinfo_watch_poll(struct file *file,
+ struct poll_table_struct *pollt)
{
struct gpio_chardev_data *cdev = file->private_data;
__poll_t events = 0;
+ guard(rwsem_read)(&cdev->gdev->sem);
+
if (!cdev->gdev->chip)
return EPOLLHUP | EPOLLERR;
@@ -2697,17 +2608,8 @@ static __poll_t lineinfo_watch_poll_unlocked(struct file *file,
return events;
}
-static __poll_t lineinfo_watch_poll(struct file *file,
- struct poll_table_struct *pollt)
-{
- struct gpio_chardev_data *cdev = file->private_data;
-
- return call_poll_locked(file, pollt, cdev->gdev,
- lineinfo_watch_poll_unlocked);
-}
-
-static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
- size_t count, loff_t *off)
+static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
+ size_t count, loff_t *off)
{
struct gpio_chardev_data *cdev = file->private_data;
struct gpio_v2_line_info_changed event;
@@ -2715,6 +2617,8 @@ static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
int ret;
size_t event_size;
+ guard(rwsem_read)(&cdev->gdev->sem);
+
if (!cdev->gdev->chip)
return -ENODEV;
@@ -2777,15 +2681,6 @@ static ssize_t lineinfo_watch_read_unlocked(struct file *file, char __user *buf,
return bytes_read;
}
-static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
- size_t count, loff_t *off)
-{
- struct gpio_chardev_data *cdev = file->private_data;
-
- return call_read_locked(file, buf, count, off, cdev->gdev,
- lineinfo_watch_read_unlocked);
-}
-
/**
* gpio_chrdev_open() - open the chardev for ioctl operations
* @inode: inode for this chardev
@@ -2799,17 +2694,15 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
struct gpio_chardev_data *cdev;
int ret = -ENOMEM;
- down_read(&gdev->sem);
+ guard(rwsem_read)(&gdev->sem);
/* Fail on open if the backing gpiochip is gone */
- if (!gdev->chip) {
- ret = -ENODEV;
- goto out_unlock;
- }
+ if (!gdev->chip)
+ return -ENODEV;
cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
if (!cdev)
- goto out_unlock;
+ return -ENODEV;
cdev->watched_lines = bitmap_zalloc(gdev->chip->ngpio, GFP_KERNEL);
if (!cdev->watched_lines)
@@ -2838,8 +2731,6 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
if (ret)
goto out_unregister_device_notifier;
- up_read(&gdev->sem);
-
return ret;
out_unregister_device_notifier:
@@ -2853,8 +2744,6 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
bitmap_free(cdev->watched_lines);
out_free_cdev:
kfree(cdev);
-out_unlock:
- up_read(&gdev->sem);
return ret;
}
--
2.39.2
On Thu, Dec 21, 2023 at 2:21 AM Kent Gibson <[email protected]> wrote:
>
> While the GPIO cdev gpio_ioctl() call is in progress, the kernel can
> call gpiochip_remove() which will set gdev->chip to NULL, after which
> any subsequent access will cause a crash.
>
> gpio_ioctl() was overlooked by the previous fix to protect syscalls
> (bdbbae241a04), so add protection for that.
>
> Fixes: bdbbae241a04 ("gpiolib: protect the GPIO device against being dropped while in use by user-space")
> Fixes: d7c51b47ac11 ("gpio: userspace ABI for reading/writing GPIO lines")
> Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
> Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
> Signed-off-by: Kent Gibson <[email protected]>
> ---
> drivers/gpio/gpiolib-cdev.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 744734405912..9155c54acc1e 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2598,10 +2598,7 @@ static int lineinfo_unwatch(struct gpio_chardev_data *cdev, void __user *ip)
> return 0;
> }
>
> -/*
> - * gpio_ioctl() - ioctl handler for the GPIO chardev
> - */
> -static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +static long gpio_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned long arg)
> {
> struct gpio_chardev_data *cdev = file->private_data;
> struct gpio_device *gdev = cdev->gdev;
> @@ -2638,6 +2635,17 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> }
> }
>
> +/*
> + * gpio_ioctl() - ioctl handler for the GPIO chardev
> + */
> +static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct gpio_chardev_data *cdev = file->private_data;
> +
> + return call_ioctl_locked(file, cmd, arg, cdev->gdev,
> + gpio_ioctl_unlocked);
> +}
> +
> #ifdef CONFIG_COMPAT
> static long gpio_ioctl_compat(struct file *file, unsigned int cmd,
> unsigned long arg)
> --
> 2.39.2
>
I applied this. I'll send it upstream tomorrow and once it's in
master, I'll pick up the rest on Monday.
Bart
On Thu, Dec 21, 2023 at 09:20:40AM +0800, Kent Gibson wrote:
> Replace the wrapping functions that inhibit removal of the gpio chip
GPIO
> with equivalent guards.
--
With Best Regards,
Andy Shevchenko
On Thu, Dec 21, 2023 at 05:48:52PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 21, 2023 at 09:20:40AM +0800, Kent Gibson wrote:
> > Replace the wrapping functions that inhibit removal of the gpio chip
>
> GPIO
>
Bart, I don't care either way and not enough to respin a v3.
If it bothers you could you fix it on the way in?
That is if you aren't too busy reversing xmas trees ;-).
Thanks,
Kent.
On Fri, Dec 22, 2023 at 1:47 AM Kent Gibson <[email protected]> wrote:
>
> On Thu, Dec 21, 2023 at 05:48:52PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 21, 2023 at 09:20:40AM +0800, Kent Gibson wrote:
> > > Replace the wrapping functions that inhibit removal of the gpio chip
> >
> > GPIO
> >
>
> Bart, I don't care either way and not enough to respin a v3.
> If it bothers you could you fix it on the way in?
>
Sure!
> That is if you aren't too busy reversing xmas trees ;-).
>
Joke's on you, I actually do find them easier to read and try to use
them everywhere in new code I write. :)
Bart
> Thanks,
> Kent.
>
On Fri, Dec 22, 2023 at 03:05:41PM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 22, 2023 at 1:47 AM Kent Gibson <[email protected]> wrote:
> >
> > On Thu, Dec 21, 2023 at 05:48:52PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 21, 2023 at 09:20:40AM +0800, Kent Gibson wrote:
> > > > Replace the wrapping functions that inhibit removal of the gpio chip
> > >
> > > GPIO
> > >
> >
> > Bart, I don't care either way and not enough to respin a v3.
> > If it bothers you could you fix it on the way in?
> >
>
> Sure!
>
> > That is if you aren't too busy reversing xmas trees ;-).
> >
>
> Joke's on you, I actually do find them easier to read and try to use
> them everywhere in new code I write. :)
>
Oh, I agree - it is more readable. It just seems very timely that Andy
keeps griping about them not being inverted.
Cheers,
Kent.
On Thu, Dec 21, 2023 at 2:20 AM Kent Gibson <[email protected]> wrote:
>
> This series contains some tidying up of gpiolib-cdev following the
> recent adoption of guard().
>
> The first patch is a fix to protect gpio_ioctl() from having the
> gpio chip removed while the ioctl is in progress.
>
> The next couple of patches are minor fixes inspired by recent
> submissions and reviews for gpiolib.c.
>
> Patch 2 adds a missing include.
>
> Patch 3 switches allocation of struct linereq from kzalloc() to
> kvzalloc() as it can be larger than one page - even more so after the
> recent relocation of debounce_period_us.
>
> The final two patches replace wrapper functions with guards.
>
> Patch 4 tidies up the functions that use a guard on the linereq
> config_mutex.
>
> Patch 5 tidies up the functions that use a guard on the gpio_device.
>
> Changes v1 -> v2:
> - add patch 1 to protect gpio_ioctl() from chip removal
> - improve commit comment (patch 3)
> - use guard(rwsem_read) rather than rolling our own (patch 5)
>
> Cheers,
> Kent.
>
> Kent Gibson (5):
> gpiolib: cdev: add gpio_device locking wrapper around gpio_ioctl()
> gpiolib: cdev: include overflow.h
> gpiolib: cdev: allocate linereq using kvzalloc()
> gpiolib: cdev: replace locking wrappers for config_mutex with guards
> gpiolib: cdev: replace locking wrappers for gpio_device with guards
>
> drivers/gpio/gpiolib-cdev.c | 257 ++++++++++--------------------------
> 1 file changed, 70 insertions(+), 187 deletions(-)
>
> --
> 2.39.2
>
I applied the remaining patches, thanks.
Bart