* [PATCH 0/2] uio: some fixes
@ 2012-12-06 12:44 Benedikt Spranger
2012-12-06 12:44 ` [PATCH 1/2] uio: be aware of open(), mmap(), close() Benedikt Spranger
2012-12-06 12:44 ` [PATCH 2/2] uio: avoid module unloding of in module created UIO devices Benedikt Spranger
0 siblings, 2 replies; 5+ messages in thread
From: Benedikt Spranger @ 2012-12-06 12:44 UTC (permalink / raw)
To: linux-kernel; +Cc: hjk, gregkh, Alexander.Frank, Benedikt Spranger
Using uio_pdrv_genirq can cause a system hangup due to accessing mmaped memory,
since the pm_runtime_put_sync() call in uio_release can disable clocks.
Also unloading a module wich creates/removes UIO devices can cause a system
crash. Track mappings and get a module reference to avoid this.
Benedikt Spranger (2):
uio: be aware of open(), mmap(), close()
uio: avoid module unloding of in module created UIO devices
drivers/uio/uio.c | 67 ++++++++++++++++++++++++++++++++---------
drivers/uio/uio_pdrv_genirq.c | 4 +--
include/linux/uio_driver.h | 5 +--
3 files changed, 57 insertions(+), 19 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] uio: be aware of open(), mmap(), close()
2012-12-06 12:44 [PATCH 0/2] uio: some fixes Benedikt Spranger
@ 2012-12-06 12:44 ` Benedikt Spranger
2012-12-06 22:28 ` Hans J. Koch
2012-12-06 12:44 ` [PATCH 2/2] uio: avoid module unloding of in module created UIO devices Benedikt Spranger
1 sibling, 1 reply; 5+ messages in thread
From: Benedikt Spranger @ 2012-12-06 12:44 UTC (permalink / raw)
To: linux-kernel; +Cc: hjk, gregkh, Alexander.Frank, Benedikt Spranger
After open(), mmap(), close() the mmaped pointer is valid. Removing the
underlaying module causes a Oops or other strange effects. Keep all structures
valid till the last user is gone.
Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
---
drivers/uio/uio.c | 59 ++++++++++++++++++++++++++++++-----------
drivers/uio/uio_pdrv_genirq.c | 4 +--
include/linux/uio_driver.h | 4 +--
3 files changed, 48 insertions(+), 19 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..b96499a 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -433,8 +433,22 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id)
struct uio_listener {
struct uio_device *dev;
s32 event_count;
+ struct kref kref;
};
+static void uio_release_listener(struct kref *kref)
+{
+ struct uio_listener *listener = container_of(kref, struct uio_listener,
+ kref);
+ struct uio_device *idev = listener->dev;
+
+ if (idev->info->release)
+ idev->info->release(idev->info);
+
+ module_put(idev->owner);
+ kfree(listener);
+}
+
static int uio_open(struct inode *inode, struct file *filep)
{
struct uio_device *idev;
@@ -465,10 +479,13 @@ static int uio_open(struct inode *inode, struct file *filep)
filep->private_data = listener;
if (idev->info->open) {
- ret = idev->info->open(idev->info, inode);
+ ret = idev->info->open(idev->info);
if (ret)
goto err_infoopen;
}
+
+ kref_init(&listener->kref);
+
return 0;
err_infoopen:
@@ -491,16 +508,11 @@ static int uio_fasync(int fd, struct file *filep, int on)
static int uio_release(struct inode *inode, struct file *filep)
{
- int ret = 0;
struct uio_listener *listener = filep->private_data;
- struct uio_device *idev = listener->dev;
- if (idev->info->release)
- ret = idev->info->release(idev->info, inode);
+ kref_put(&listener->kref, uio_release_listener);
- module_put(idev->owner);
- kfree(listener);
- return ret;
+ return 0;
}
static unsigned int uio_poll(struct file *filep, poll_table *wait)
@@ -605,19 +617,22 @@ static int uio_find_mem_index(struct vm_area_struct *vma)
static void uio_vma_open(struct vm_area_struct *vma)
{
- struct uio_device *idev = vma->vm_private_data;
- idev->vma_count++;
+ struct uio_listener *listener = vma->vm_private_data;
+
+ kref_get(&listener->kref);
}
static void uio_vma_close(struct vm_area_struct *vma)
{
- struct uio_device *idev = vma->vm_private_data;
- idev->vma_count--;
+ struct uio_listener *listener = vma->vm_private_data;
+
+ kref_put(&listener->kref, uio_release_listener);
}
static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
{
- struct uio_device *idev = vma->vm_private_data;
+ struct uio_listener *listener = vma->vm_private_data;
+ struct uio_device *idev = listener->dev;
struct page *page;
unsigned long offset;
@@ -646,20 +661,34 @@ static const struct vm_operations_struct uio_vm_ops = {
.fault = uio_vma_fault,
};
+static const struct vm_operations_struct uio_phys_ops = {
+ .open = uio_vma_open,
+ .close = uio_vma_close,
+};
+
static int uio_mmap_physical(struct vm_area_struct *vma)
{
struct uio_device *idev = vma->vm_private_data;
int mi = uio_find_mem_index(vma);
+ int ret;
+
if (mi < 0)
return -EINVAL;
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+ vma->vm_ops = &uio_phys_ops;
- return remap_pfn_range(vma,
+ ret = remap_pfn_range(vma,
vma->vm_start,
idev->info->mem[mi].addr >> PAGE_SHIFT,
vma->vm_end - vma->vm_start,
vma->vm_page_prot);
+ if (ret)
+ return ret;
+
+ uio_vma_open(vma);
+
+ return 0;
}
static int uio_mmap_logical(struct vm_area_struct *vma)
@@ -681,7 +710,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
if (vma->vm_end < vma->vm_start)
return -EINVAL;
- vma->vm_private_data = idev;
+ vma->vm_private_data = listener;
mi = uio_find_mem_index(vma);
if (mi < 0)
diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
index 42202cd..a4e32fa 100644
--- a/drivers/uio/uio_pdrv_genirq.c
+++ b/drivers/uio/uio_pdrv_genirq.c
@@ -37,7 +37,7 @@ struct uio_pdrv_genirq_platdata {
struct platform_device *pdev;
};
-static int uio_pdrv_genirq_open(struct uio_info *info, struct inode *inode)
+static int uio_pdrv_genirq_open(struct uio_info *info)
{
struct uio_pdrv_genirq_platdata *priv = info->priv;
@@ -46,7 +46,7 @@ static int uio_pdrv_genirq_open(struct uio_info *info, struct inode *inode)
return 0;
}
-static int uio_pdrv_genirq_release(struct uio_info *info, struct inode *inode)
+static int uio_pdrv_genirq_release(struct uio_info *info)
{
struct uio_pdrv_genirq_platdata *priv = info->priv;
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 1ad4724..1bc6493 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -92,8 +92,8 @@ struct uio_info {
void *priv;
irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
- int (*open)(struct uio_info *info, struct inode *inode);
- int (*release)(struct uio_info *info, struct inode *inode);
+ int (*open)(struct uio_info *info);
+ void (*release)(struct uio_info *info);
int (*irqcontrol)(struct uio_info *info, s32 irq_on);
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] uio: avoid module unloding of in module created UIO devices
2012-12-06 12:44 [PATCH 0/2] uio: some fixes Benedikt Spranger
2012-12-06 12:44 ` [PATCH 1/2] uio: be aware of open(), mmap(), close() Benedikt Spranger
@ 2012-12-06 12:44 ` Benedikt Spranger
2012-12-06 22:25 ` Hans J. Koch
1 sibling, 1 reply; 5+ messages in thread
From: Benedikt Spranger @ 2012-12-06 12:44 UTC (permalink / raw)
To: linux-kernel; +Cc: hjk, gregkh, Alexander.Frank, Benedikt Spranger
A kernel module can create a uio device. Get a reference to the module, if the
UIO device is in use. Otherwise the device can be removed and a uio write or an
access to am mmaped memory can cause a kernel Oops or other strange effects.
Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
---
drivers/uio/uio.c | 8 ++++++++
include/linux/uio_driver.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index b96499a..3b62da0 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -446,6 +446,7 @@ static void uio_release_listener(struct kref *kref)
idev->info->release(idev->info);
module_put(idev->owner);
+ module_put(idev->info->owner);
kfree(listener);
}
@@ -468,6 +469,11 @@ static int uio_open(struct inode *inode, struct file *filep)
goto out;
}
+ if (!try_module_get(idev->info->owner)) {
+ ret = -ENODEV;
+ goto err_get_module;
+ }
+
listener = kmalloc(sizeof(*listener), GFP_KERNEL);
if (!listener) {
ret = -ENOMEM;
@@ -493,6 +499,8 @@ err_infoopen:
err_alloc_listener:
module_put(idev->owner);
+err_get_module:
+ module_put(idev->owner);
out:
return ret;
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 1bc6493..2862de23 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -95,6 +95,7 @@ struct uio_info {
int (*open)(struct uio_info *info);
void (*release)(struct uio_info *info);
int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+ struct module *owner;
};
extern int __must_check
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] uio: avoid module unloding of in module created UIO devices
2012-12-06 12:44 ` [PATCH 2/2] uio: avoid module unloding of in module created UIO devices Benedikt Spranger
@ 2012-12-06 22:25 ` Hans J. Koch
0 siblings, 0 replies; 5+ messages in thread
From: Hans J. Koch @ 2012-12-06 22:25 UTC (permalink / raw)
To: Benedikt Spranger; +Cc: linux-kernel, hjk, gregkh, Alexander.Frank
On Thu, Dec 06, 2012 at 01:44:57PM +0100, Benedikt Spranger wrote:
> A kernel module can create a uio device. Get a reference to the module, if the
> UIO device is in use. Otherwise the device can be removed and a uio write or an
> access to am mmaped memory can cause a kernel Oops or other strange effects.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/uio/uio.c | 8 ++++++++
> include/linux/uio_driver.h | 1 +
> 2 files changed, 9 insertions(+)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index b96499a..3b62da0 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -446,6 +446,7 @@ static void uio_release_listener(struct kref *kref)
> idev->info->release(idev->info);
>
> module_put(idev->owner);
> + module_put(idev->info->owner);
> kfree(listener);
> }
>
> @@ -468,6 +469,11 @@ static int uio_open(struct inode *inode, struct file *filep)
> goto out;
> }
>
> + if (!try_module_get(idev->info->owner)) {
> + ret = -ENODEV;
> + goto err_get_module;
> + }
> +
> listener = kmalloc(sizeof(*listener), GFP_KERNEL);
> if (!listener) {
> ret = -ENOMEM;
> @@ -493,6 +499,8 @@ err_infoopen:
>
> err_alloc_listener:
> module_put(idev->owner);
> +err_get_module:
> + module_put(idev->owner);
>
> out:
> return ret;
> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> index 1bc6493..2862de23 100644
> --- a/include/linux/uio_driver.h
> +++ b/include/linux/uio_driver.h
> @@ -95,6 +95,7 @@ struct uio_info {
> int (*open)(struct uio_info *info);
> void (*release)(struct uio_info *info);
> int (*irqcontrol)(struct uio_info *info, s32 irq_on);
> + struct module *owner;
Where is that needed? I won't add a new element to struct uio_info without
an in-kernel user.
Thanks,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] uio: be aware of open(), mmap(), close()
2012-12-06 12:44 ` [PATCH 1/2] uio: be aware of open(), mmap(), close() Benedikt Spranger
@ 2012-12-06 22:28 ` Hans J. Koch
0 siblings, 0 replies; 5+ messages in thread
From: Hans J. Koch @ 2012-12-06 22:28 UTC (permalink / raw)
To: Benedikt Spranger; +Cc: linux-kernel, hjk, gregkh, Alexander.Frank
On Thu, Dec 06, 2012 at 01:44:56PM +0100, Benedikt Spranger wrote:
> After open(), mmap(), close() the mmaped pointer is valid. Removing the
> underlaying module causes a Oops or other strange effects. Keep all structures
> valid till the last user is gone.
>
> Signed-off-by: Benedikt Spranger <b.spranger@linutronix.de>
> ---
> drivers/uio/uio.c | 59 ++++++++++++++++++++++++++++++-----------
> drivers/uio/uio_pdrv_genirq.c | 4 +--
> include/linux/uio_driver.h | 4 +--
> 3 files changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..b96499a 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -433,8 +433,22 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id)
> struct uio_listener {
> struct uio_device *dev;
> s32 event_count;
> + struct kref kref;
> };
>
> +static void uio_release_listener(struct kref *kref)
> +{
> + struct uio_listener *listener = container_of(kref, struct uio_listener,
> + kref);
> + struct uio_device *idev = listener->dev;
> +
> + if (idev->info->release)
> + idev->info->release(idev->info);
> +
> + module_put(idev->owner);
> + kfree(listener);
> +}
> +
> static int uio_open(struct inode *inode, struct file *filep)
> {
> struct uio_device *idev;
> @@ -465,10 +479,13 @@ static int uio_open(struct inode *inode, struct file *filep)
> filep->private_data = listener;
>
> if (idev->info->open) {
> - ret = idev->info->open(idev->info, inode);
> + ret = idev->info->open(idev->info);
> if (ret)
> goto err_infoopen;
> }
> +
> + kref_init(&listener->kref);
> +
> return 0;
>
> err_infoopen:
> @@ -491,16 +508,11 @@ static int uio_fasync(int fd, struct file *filep, int on)
>
> static int uio_release(struct inode *inode, struct file *filep)
> {
> - int ret = 0;
> struct uio_listener *listener = filep->private_data;
> - struct uio_device *idev = listener->dev;
>
> - if (idev->info->release)
> - ret = idev->info->release(idev->info, inode);
> + kref_put(&listener->kref, uio_release_listener);
>
> - module_put(idev->owner);
> - kfree(listener);
> - return ret;
> + return 0;
> }
>
> static unsigned int uio_poll(struct file *filep, poll_table *wait)
> @@ -605,19 +617,22 @@ static int uio_find_mem_index(struct vm_area_struct *vma)
>
> static void uio_vma_open(struct vm_area_struct *vma)
> {
> - struct uio_device *idev = vma->vm_private_data;
> - idev->vma_count++;
> + struct uio_listener *listener = vma->vm_private_data;
> +
> + kref_get(&listener->kref);
> }
>
> static void uio_vma_close(struct vm_area_struct *vma)
> {
> - struct uio_device *idev = vma->vm_private_data;
> - idev->vma_count--;
> + struct uio_listener *listener = vma->vm_private_data;
> +
> + kref_put(&listener->kref, uio_release_listener);
> }
>
> static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> {
> - struct uio_device *idev = vma->vm_private_data;
> + struct uio_listener *listener = vma->vm_private_data;
> + struct uio_device *idev = listener->dev;
> struct page *page;
> unsigned long offset;
>
> @@ -646,20 +661,34 @@ static const struct vm_operations_struct uio_vm_ops = {
> .fault = uio_vma_fault,
> };
>
> +static const struct vm_operations_struct uio_phys_ops = {
> + .open = uio_vma_open,
> + .close = uio_vma_close,
> +};
> +
> static int uio_mmap_physical(struct vm_area_struct *vma)
> {
> struct uio_device *idev = vma->vm_private_data;
> int mi = uio_find_mem_index(vma);
> + int ret;
> +
> if (mi < 0)
> return -EINVAL;
>
> vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + vma->vm_ops = &uio_phys_ops;
>
> - return remap_pfn_range(vma,
> + ret = remap_pfn_range(vma,
> vma->vm_start,
> idev->info->mem[mi].addr >> PAGE_SHIFT,
> vma->vm_end - vma->vm_start,
> vma->vm_page_prot);
> + if (ret)
> + return ret;
> +
> + uio_vma_open(vma);
> +
> + return 0;
> }
>
> static int uio_mmap_logical(struct vm_area_struct *vma)
> @@ -681,7 +710,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
> if (vma->vm_end < vma->vm_start)
> return -EINVAL;
>
> - vma->vm_private_data = idev;
> + vma->vm_private_data = listener;
>
> mi = uio_find_mem_index(vma);
> if (mi < 0)
> diff --git a/drivers/uio/uio_pdrv_genirq.c b/drivers/uio/uio_pdrv_genirq.c
> index 42202cd..a4e32fa 100644
> --- a/drivers/uio/uio_pdrv_genirq.c
> +++ b/drivers/uio/uio_pdrv_genirq.c
> @@ -37,7 +37,7 @@ struct uio_pdrv_genirq_platdata {
> struct platform_device *pdev;
> };
>
> -static int uio_pdrv_genirq_open(struct uio_info *info, struct inode *inode)
> +static int uio_pdrv_genirq_open(struct uio_info *info)
> {
> struct uio_pdrv_genirq_platdata *priv = info->priv;
>
> @@ -46,7 +46,7 @@ static int uio_pdrv_genirq_open(struct uio_info *info, struct inode *inode)
> return 0;
> }
>
> -static int uio_pdrv_genirq_release(struct uio_info *info, struct inode *inode)
> +static int uio_pdrv_genirq_release(struct uio_info *info)
> {
> struct uio_pdrv_genirq_platdata *priv = info->priv;
>
> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> index 1ad4724..1bc6493 100644
> --- a/include/linux/uio_driver.h
> +++ b/include/linux/uio_driver.h
> @@ -92,8 +92,8 @@ struct uio_info {
> void *priv;
> irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
> int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
> - int (*open)(struct uio_info *info, struct inode *inode);
> - int (*release)(struct uio_info *info, struct inode *inode);
> + int (*open)(struct uio_info *info);
> + void (*release)(struct uio_info *info);
Interface changes like that need an explanation and want to go to
a separate patch.
Thanks,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-12-06 22:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-06 12:44 [PATCH 0/2] uio: some fixes Benedikt Spranger
2012-12-06 12:44 ` [PATCH 1/2] uio: be aware of open(), mmap(), close() Benedikt Spranger
2012-12-06 22:28 ` Hans J. Koch
2012-12-06 12:44 ` [PATCH 2/2] uio: avoid module unloding of in module created UIO devices Benedikt Spranger
2012-12-06 22:25 ` Hans J. Koch
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).