lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 18 Sep 2017 09:50:21 +0100 From: Lee Jones <lee.jones@...aro.org> To: Martin Kaiser <martin@...ser.cx> Cc: kernel@...gutronix.de, linux-kernel@...r.kernel.org Subject: Re: [PATCH 2/2] mfd: fsl-imx25: clean up irq settings during removal On Thu, 14 Sep 2017, Martin Kaiser wrote: > Thus wrote Lee Jones (lee.jones@...aro.org): > > > On Tue, 12 Sep 2017, Martin Kaiser wrote: > > > > When fsl-imx25-tsadc is compiled as a module, unloading and reloading > > > the module will lead to a crash. > > > > Add a removal function which clears the irq handler and removes the irq > > > domain. With this cleanup in place, it's possible to unload and reload > > > the module. > > > More information please. > > > What is it specifically that causes the crash? > > I tested using a touchscreen and compiled both > drivers/input/touchscreen/fsl-imx25-tcq.c and > drivers/mfd/fsl-imx25-tsadc.c > as modules. > > I got the crash after running > insmod fsl-imx25-tcq.ko > insmod fsl-imx25-tsadc.ko > rmmod fsl-imx25-tsadc.ko > insmod fsl-imx25-tsadc.ko > > [ 133.594246] Unable to handle kernel paging request at virtual address bf005430 > [ 133.601685] pgd = d3b90000 > [ 133.604435] [bf005430] *pgd=93b61811, *pte=00000000, *ppte=00000000 > [ 133.610902] Internal error: Oops: 7 [#1] ARM > [ 133.615208] Modules linked in: fsl_imx25_tsadc(+) fsl_imx25_tcq [last unloaded: fsl_imx25_tsadc] > [ 133.624078] CPU: 0 PID: 173 Comm: insmod Not tainted 4.13.0-next-20170911+ #132 > [ 133.631413] Hardware name: Freescale i.MX25 (Device Tree Support) > [ 133.637537] task: d3b64000 task.stack: d3a64000 > [ 133.642131] PC is at irq_find_matching_fwspec+0x40/0x108 > [ 133.647484] LR is at irq_find_matching_fwspec+0x30/0x108 > [ 133.652826] pc : [<c004dfac>] lr : [<c004df9c>] psr: 20000013 > [ 133.659116] sp : d3a65bb0 ip : d3a65bb0 fp : d3a65bd4 > [ 133.664362] r10: 00000000 r9 : c03cac4c r8 : d3a65c20 > [ 133.669612] r7 : 00000000 r6 : c0ab5a58 r5 : d3d700dc r4 : d3b80000 > [ 133.676167] r3 : bf00542c r2 : 40000013 r1 : d3b64000 r0 : c0ab5a4c > [ 133.682721] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > [ 133.689883] Control: 0005317f Table: 93b90000 DAC: 00000051 > [ 133.695655] Process insmod (pid: 173, stack limit = 0xd3a64190) > ... > [ 133.993772] Backtrace: > [ 133.996307] [<c004df6c>] (irq_find_matching_fwspec) from [<c028d5ec>] (of_irq_get+0x58/0x74) > [ 134.004798] r9:d3d737ec r8:d3948000 r7:d3948010 r6:d3b6cb10 r5:00000000 r4:d3d700dc > [ 134.012607] [<c028d594>] (of_irq_get) from [<c01ff970>] (platform_get_irq+0x48/0xc8) > [ 134.020375] r4:d3948000 > [ 134.022990] [<c01ff928>] (platform_get_irq) from [<bf00e33c>] (mx25_tsadc_probe+0x220/0x2f4 [fsl_imx25_tsadc]) > [ 134.033019] r5:00000012 r4:00000000 > [ 134.036663] [<bf00e11c>] (mx25_tsadc_probe [fsl_imx25_tsadc]) from [<c01ff658>] (platform_drv_probe+0x60/0xb0) > [ 134.046711] r9:00000008 r8:00000000 r7:c0b15680 r6:bf00e7b0 r5:d3948010 r4:bf00e11c > [ 134.054504] [<c01ff5f8>] (platform_drv_probe) from [<c01fd5fc>] (driver_probe_device+0x204/0x470) > [ 134.063414] r6:00000000 r5:bf00e7b0 r4:d3948010 r3:c01ff5f8 > [ 134.069119] [<c01fd3f8>] (driver_probe_device) from [<c01fd940>] (__driver_attach+0xd8/0x118) > [ 134.077691] r9:bf00e84c r8:c0af6078 r7:c0ad8ee0 r6:bf00e7b0 r5:d3948044 r4:d3948010 > [ 134.085483] [<c01fd868>] (__driver_attach) from [<c01fb734>] (bus_for_each_dev+0x7c/0xa0) > > > irq_find_matching_fwspec() loops over all registered irq domains, I guess the > crash happens inside this loop. The irq domain is still registered from last > time the module was loaded but the pointer to its operations is invalid after > the module was unloaded. Great. Please condense this information as much as possible and place it in the commit log. > My hope was that removing the irq domain along with module removal would fix > the crash in a proper way. > > > Why is this code not required? What's stopping this causing a leak? > > Not sure I got your point here. Never mind. This was due to an incorrect assumption I made. You can ignore this. > My assumption is that (at least for platform drivers), the removal function is > called only when the previous probe was successful. platform_get_irq() should > then get us the same irq we got in the probe function and we can remove the > handler and the irq domain. > > Needless to say, I'm happy to update and re-test the patch if required. > > Best regards, > > Martin -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists