[RFC] omap3isp: prevent releasing MC too early

Commit Message

Sakari Ailus Dec. 16, 2016, 11:44 a.m. UTC

Hi folks,

On Fri, Dec 16, 2016 at 10:21:25AM +0200, Sakari Ailus wrote:
...
> At least some of the issues the patches claim to fix are really not fixed
> either. They are just made slightly less likely to accidentally stumble
> upon.

I couldn't immediately trigger this with the current mainline kernel as the
remaining time window isn't very large. However making this trivial-looking
change:



And then unbinding the omap3-isp driver will immediately produce this. The
same would take place on any driver implementing the Media controller
interface. What happens there is that the memory backing the media device
has been released while the IOCTL call was in progress. This can happen
because device removal and IOCTL calls are not serialised nor the media
device is reference counted --- the latter of which I believe is the right
thing to do:

[  105.756408] Unable to handle kernel NULL pointer dereference at virtual address 00000140
[  105.756561] pgd = eda94000
[  105.756591] [00000140] *pgd=adaf8831, *pte=00000000, *ppte=00000000
[  105.756683] Internal error: Oops: 17 [#1] PREEMPT ARM
[  105.756744] Modules linked in: smiapp smiapp_pll omap3_isp videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_core v4l2_common videodev media
[  105.756927] CPU: 0 PID: 2276 Comm: media-ctl Not tainted 4.9.0-rc6-00494-g9244e38-dirty #830
[  105.757019] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[  105.757080] task: ed970380 task.stack: eda5e000
[  105.757141] PC is at __lock_acquire+0x94/0x1868
[  105.757202] LR is at lock_acquire+0x70/0x90
[  105.757263] pc : [<c015e9a8>]    lr : [<c01601ec>]    psr: 20000093
               sp : eda5fcc0  ip : eda5e000  fp : 00000000
[  105.757354] r10: 00000001  r9 : 00000000  r8 : 0002715c
[  105.757415] r7 : 00000140  r6 : 00000001  r5 : 60000013  r4 : ed970380
[  105.757476] r3 : 00000001  r2 : 00000001  r1 : 00000000  r0 : 00000140
[  105.757537] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  105.757629] Control: 10c5387d  Table: ada94019  DAC: 00000051
[  105.757690] Process media-ctl (pid: 2276, stack limit = 0xeda5e208)
[  105.757751] Stack: (0xeda5fcc0 to 0xeda60000)
[  105.757812] fcc0: ed970868 00000001 a409df10 c355d08b c0c75714 c01602f4 60000013 00000001
[  105.757904] fce0: ee1c9514 c0839b48 ee1c9518 effed360 ed970868 60000013 c0833e74 c0839b48
[  105.757995] fd00: effed360 effed360 c0e9166c ee1c951c ee1c9518 00000000 60000013 00000001
[  105.758087] fd20: 00000000 0002715c eda5e000 00000000 00000000 c01601ec 00000001 00000000
[  105.758178] fd40: 00000000 bf000e78 00000000 00000000 0000010c bf000e78 ed970380 c04ed214
[  105.758270] fd60: 00000001 00000000 bf000e78 c015aa50 ed9703b0 ed9703b0 c0830d50 00000000
[  105.758361] fd80: ed970848 c0830d60 ed970380 ed970380 c0830d60 c01602f4 c0e76744 00000051
[  105.758483] fda0: eda5fdd0 00000000 0002715c eda5fdd0 bf002e28 c1007c01 00000000 0002715c
[  105.758575] fdc0: eda5e000 00000000 00000000 bf000e78 00000001 50414d4f 53492033 43432050
[  105.758666] fde0: 00003250 00000000 00000000 00000000 00000000 00020000 00000000 00000000
[  105.758758] fe00: 00000000 00010002 00000000 00000000 00000000 00000000 00000051 00000007
[  105.758850] fe20: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[  105.758941] fe40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[  105.759033] fe60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[  105.759124] fe80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[  105.759216] fea0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[  105.759307] fec0: 00000000 00000000 00000000 00000000 ed967c80 bf000ddc ed967c80 0002715c
[  105.759399] fee0: c1007c01 00000003 00000000 bf001808 0002715c ed967c80 00000003 ed9fece0
[  105.759490] ff00: 00000003 c01ea4c4 0002715c c01eb4a8 00000000 00000000 c01fba18 ffffffff
[  105.759582] ff20: 00000001 00000000 ee84f010 ed89e440 00000000 ee2654e8 00000010 c01dacdc
[  105.759674] ff40: 00000000 00000000 00000000 ed89e538 ed8a2b50 ed9707a0 ed970380 00000000
[  105.759765] ff60: eda5ff7c eda5ff84 ed967c80 ed967c80 0002715c c1007c01 00000003 eda5e000
[  105.759857] ff80: 00000000 c01eb560 0002715c 00027008 00000000 00000036 c0107d84 eda5e000
[  105.759948] ffa0: 00000000 c0107be0 0002715c 00027008 00000003 c1007c01 0002715c 80000000
[  105.760070] ffc0: 0002715c 00027008 00000000 00000036 00027158 00000000 b6f71000 00000000
[  105.760162] ffe0: 00026b08 be9cf85c 000126a4 b6eaaa5c 20000010 00000003 fda7fddf fdbdffba
[  105.760253] [<c015e9a8>] (__lock_acquire) from [<c01601ec>] (lock_acquire+0x70/0x90)
[  105.760375] [<c01601ec>] (lock_acquire) from [<c04ed214>] (mutex_lock_nested+0x54/0x3f4)
[  105.760528] [<c04ed214>] (mutex_lock_nested) from [<bf000e78>] (media_device_ioctl+0x9c/0x120 [media])
[  105.760681] [<bf000e78>] (media_device_ioctl [media]) from [<bf001808>] (media_ioctl+0x54/0x60 [media])
[  105.760803] [<bf001808>] (media_ioctl [media]) from [<c01ea4c4>] (vfs_ioctl+0x18/0x30)
[  105.760894] [<c01ea4c4>] (vfs_ioctl) from [<c01eb4a8>] (do_vfs_ioctl+0x90c/0x974)
[  105.760986] [<c01eb4a8>] (do_vfs_ioctl) from [<c01eb560>] (SyS_ioctl+0x50/0x6c)
[  105.761108] [<c01eb560>] (SyS_ioctl) from [<c0107be0>] (ret_fast_syscall+0x0/0x1c)
[  105.761199] Code: e59f3f58 e593300c e3530000 0a000003 (e5972000) 
[  105.761260] ---[ end trace bdb47e9a97e34f03 ]---

Patch

diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
index f2772ba..6ec4125 100644
--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c
@@ -32,6 +32,7 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -128,6 +129,8 @@  __media_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
        if (!media_devnode_is_registered(devnode))
                return -EIO;
 
+       msleep(1000);
+
        return ioctl_func(filp, cmd, arg);
 }
 
And calling MEDIA_IOC_ENUM_ENTITIES (for instance) in a loop (v4l-utils):

diff --git a/utils/media-ctl/libmediactl.c b/utils/media-ctl/libmediactl.c
index 1fd6525..5be3cea 100644
--- a/utils/media-ctl/libmediactl.c
+++ b/utils/media-ctl/libmediactl.c
@@ -522,7 +522,7 @@  static int media_enum_entities(struct media_device *media)
 		entity->fd = -1;
 		entity->info.id = id | MEDIA_ENT_ID_FLAG_NEXT;
 		entity->media = media;
-
+while (1)
 		ret = ioctl(media->fd, MEDIA_IOC_ENUM_ENTITIES, &entity->info);
 		if (ret < 0) {
 			ret = errno != EINVAL ? -errno : 0;