[PATCH] tw686x: Fix oops on buffer alloc failure
From: Krzysztof Hałasa The error path currently calls tw686x_video_free() which requires vc->dev to be initialized, causing a NULL dereference on uninitizalized channels. Fix this by setting the vc->dev fields for all the channels first. Fixes: f8afaa8dbc0d ("[media] tw686x: Introduce an interface to support multiple DMA modes") Signed-off-by: Krzysztof Hałasa --- drivers/media/pci/tw686x/tw686x-video.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c index 0ea8dd44026c..3a06c000f97b 100644 --- a/drivers/media/pci/tw686x/tw686x-video.c +++ b/drivers/media/pci/tw686x/tw686x-video.c @@ -1190,6 +1190,14 @@ int tw686x_video_init(struct tw686x_dev *dev) return err; } + /* Initialize vc->dev and vc->ch for the error path */ + for (ch = 0; ch < max_channels(dev); ch++) { + struct tw686x_video_channel *vc = >video_channels[ch]; + + vc->dev = dev; + vc->ch = ch; + } + for (ch = 0; ch < max_channels(dev); ch++) { struct tw686x_video_channel *vc = >video_channels[ch]; struct video_device *vdev; @@ -1198,9 +1206,6 @@ int tw686x_video_init(struct tw686x_dev *dev) spin_lock_init(>qlock); INIT_LIST_HEAD(>vidq_queued); - vc->dev = dev; - vc->ch = ch; - /* default settings */ err = tw686x_set_standard(vc, V4L2_STD_NTSC); if (err) -- 2.18.0.rc2
Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure
On 23 June 2017 at 05:18, Krzysztof Hałasawrote: > Hans Verkuil writes: > >> Any progress on this? I gather I can expect a new patch from someone? > > Well, the issue is trivial and very easy to test, though not present > on common x86 hw. That patch I've sent fixes it, but I'm not the one who > decides. If you can re-submit your patch addressing all the comments, I'd be happy to Ack it. As it stands, with the wrong subject style and without a commit log, it's a NAK on my side. -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure
Hans Verkuilwrites: > Any progress on this? I gather I can expect a new patch from someone? Well, the issue is trivial and very easy to test, though not present on common x86 hw. That patch I've sent fixes it, but I'm not the one who decides. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure
Ezequiel Garciawrites: > How about this one (untested) ? > > diff --git a/drivers/media/pci/tw686x/tw686x-video.c > b/drivers/media/pci/tw686x/tw686x-video.c > index c3fafa97b2d0..77b8d2dbd995 100644 > --- a/drivers/media/pci/tw686x/tw686x-video.c > +++ b/drivers/media/pci/tw686x/tw686x-video.c > @@ -86,6 +86,9 @@ static void tw686x_memcpy_dma_free(struct > tw686x_video_channel *vc, > struct pci_dev *pci_dev; > unsigned long flags; > > + /* Make sure this channel is initialized */ > + if (!dev) > + return; Whatever you wish. Just make sure it doesn't bomb out by default, when one happens to have such a card in his or her machine. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure
On 11 May 2017 at 04:41, Krzysztof Hałasawrote: > Ezequiel Garcia writes: > >>> + /* Initialize vc->dev and vc->ch for the error path first */ >>> + for (ch = 0; ch < max_channels(dev); ch++) { >>> + struct tw686x_video_channel *vc = >video_channels[ch]; >>> + vc->dev = dev; >>> + vc->ch = ch; >>> + } >>> + >> >> I'm not sure where is the oops this commit fixes, care to explain it to me? > > The error path apparently calls tw686x_video_free() which requires > vc->dev to be initialized. Now, the vc->dev is set for the channel being > currently initialized (unsuccesfully), but not for ones which haven't > been initialized yet. tw686x_video_free() iterates over the whole set. > > It seems it also happens in "memcpy" mode. I didn't test it before since > on my ARMv7 "memcpy" mode is unusable, it's way too slow. Also, does the > driver attempt to use consistent memory for entire buffers in this mode? Yes, memcpy mode allocates a couple P and B dma-coherent bounce buffers for each channel. > This may work on i686/x86_64 because the caches are coherent by design > and there is no difference between consistent and non-consistent RAM > (if one isn't using SWIOTLB etc). > > tw6869: PCI :07:00.0, IRQ 24, MMIO 0x110 (memcpy mode) > tw686x :07:00.0: enabling device (0140 -> 0142) > tw686x :07:00.0: dma0: unable to allocate P-buffer > Unable to handle kernel NULL pointer dereference at virtual address > PC is at _raw_spin_lock_irqsave+0x10/0x4c > LR is at tw686x_memcpy_dma_free+0x1c/0x124 > pc : [<805a8b14>]lr : [<7f04a3c0>]psr: 20010093 > sp : be915c80 ip : fp : bea1b000 > r10: r9 : fff4 r8 : b000 > r7 : r6 : 03f0 r5 : r4 : bf0e21f8 > r3 : 7f04a3a4 r2 : r1 : r0 : 20010013 > Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 4e91804a DAC: 0051 > Process udevd (pid: 88, stack limit = 0xbe914210) > (_raw_spin_lock_irqsave) from (tw686x_memcpy_dma_free+0x1c/0x124) > (tw686x_memcpy_dma_free) from (tw686x_video_free+0x50/0x78) > (tw686x_video_free) from (tw686x_video_init+0x478/0x5e8) > (tw686x_video_init) from (tw686x_probe+0x36c/0x3fc) > (tw686x_probe) from (pci_device_probe+0x88/0xf4) > (pci_device_probe) from (driver_probe_device+0x238/0x2d8) > (driver_probe_device) from (__driver_attach+0xac/0xb0) > (__driver_attach) from (bus_for_each_dev+0x6c/0xa0) > (bus_for_each_dev) from (bus_add_driver+0x1a0/0x218) > (bus_add_driver) from (driver_register+0x78/0xf8) > (driver_register) from (do_one_initcall+0x40/0x168) > (do_one_initcall) from (do_init_module+0x60/0x3a4) > (do_init_module) from (load_module+0x1c90/0x20e4) > (load_module) from (SyS_finit_module+0x8c/0x9c) > (SyS_finit_module) from (ret_fast_syscall+0x0/0x3c) > Code: e1a02000 e10f f10c0080 f592f000 (e1923f9f) > > > With the patch: > tw6869: PCI :07:00.0, IRQ 24, MMIO 0x110 (memcpy mode) > tw686x :07:00.0: enabling device (0140 -> 0142) > tw686x :07:00.0: dma0: unable to allocate P-buffer > tw686x :07:00.0: can't register video > tw686x: probe of :07:00.0 failed with error -12 Oh, I see. Thanks for the details! How about this one (untested) ? diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c index c3fafa97b2d0..77b8d2dbd995 100644 --- a/drivers/media/pci/tw686x/tw686x-video.c +++ b/drivers/media/pci/tw686x/tw686x-video.c @@ -86,6 +86,9 @@ static void tw686x_memcpy_dma_free(struct tw686x_video_channel *vc, struct pci_dev *pci_dev; unsigned long flags; + /* Make sure this channel is initialized */ + if (!dev) + return; /* Check device presence. Shouldn't really happen! */ spin_lock_irqsave(>lock, flags); pci_dev = dev->pci_dev; Also, when submitting a patch please make sure you write a commit log. Only trivial patches can have empty commit logs. The description you made in your last mail is perfect, including the oops details, the backtrace and the "With the patch" note. Doing a `git log --grep="fix.*oops"` might give you some ideas on commit logs. The subject of the patch should be: "tw686x: Fix oops on buffer alloc failure" If you `git log` on tw686x, you'll notice all the commits use "tw686x" prefix. When submitting fixes, it's useful to add a "Fixes" tag. See https://static.lwn.net/kerneldoc/process/submitting-patches.html for details. -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure
"zhaoxuegang"writes: > In my opinion, the oops occur to tw686x_free_dma(struct tw686x_video_channel > *vc, unsigned int pb). > In function tw686x_video_init, if coherent-DMA is not enough, > tw686x_alloc_dma will failed. > Then tw686x_video_free will be called. but some channel's dev is null(in > other words, vc->dev is null) Exactly. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure
Ezequiel Garciawrites: >> + /* Initialize vc->dev and vc->ch for the error path first */ >> + for (ch = 0; ch < max_channels(dev); ch++) { >> + struct tw686x_video_channel *vc = >video_channels[ch]; >> + vc->dev = dev; >> + vc->ch = ch; >> + } >> + > > I'm not sure where is the oops this commit fixes, care to explain it to me? The error path apparently calls tw686x_video_free() which requires vc->dev to be initialized. Now, the vc->dev is set for the channel being currently initialized (unsuccesfully), but not for ones which haven't been initialized yet. tw686x_video_free() iterates over the whole set. It seems it also happens in "memcpy" mode. I didn't test it before since on my ARMv7 "memcpy" mode is unusable, it's way too slow. Also, does the driver attempt to use consistent memory for entire buffers in this mode? This may work on i686/x86_64 because the caches are coherent by design and there is no difference between consistent and non-consistent RAM (if one isn't using SWIOTLB etc). tw6869: PCI :07:00.0, IRQ 24, MMIO 0x110 (memcpy mode) tw686x :07:00.0: enabling device (0140 -> 0142) tw686x :07:00.0: dma0: unable to allocate P-buffer Unable to handle kernel NULL pointer dereference at virtual address PC is at _raw_spin_lock_irqsave+0x10/0x4c LR is at tw686x_memcpy_dma_free+0x1c/0x124 pc : [<805a8b14>]lr : [<7f04a3c0>]psr: 20010093 sp : be915c80 ip : fp : bea1b000 r10: r9 : fff4 r8 : b000 r7 : r6 : 03f0 r5 : r4 : bf0e21f8 r3 : 7f04a3a4 r2 : r1 : r0 : 20010013 Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 4e91804a DAC: 0051 Process udevd (pid: 88, stack limit = 0xbe914210) (_raw_spin_lock_irqsave) from (tw686x_memcpy_dma_free+0x1c/0x124) (tw686x_memcpy_dma_free) from (tw686x_video_free+0x50/0x78) (tw686x_video_free) from (tw686x_video_init+0x478/0x5e8) (tw686x_video_init) from (tw686x_probe+0x36c/0x3fc) (tw686x_probe) from (pci_device_probe+0x88/0xf4) (pci_device_probe) from (driver_probe_device+0x238/0x2d8) (driver_probe_device) from (__driver_attach+0xac/0xb0) (__driver_attach) from (bus_for_each_dev+0x6c/0xa0) (bus_for_each_dev) from (bus_add_driver+0x1a0/0x218) (bus_add_driver) from (driver_register+0x78/0xf8) (driver_register) from (do_one_initcall+0x40/0x168) (do_one_initcall) from (do_init_module+0x60/0x3a4) (do_init_module) from (load_module+0x1c90/0x20e4) (load_module) from (SyS_finit_module+0x8c/0x9c) (SyS_finit_module) from (ret_fast_syscall+0x0/0x3c) Code: e1a02000 e10f f10c0080 f592f000 (e1923f9f) With the patch: tw6869: PCI :07:00.0, IRQ 24, MMIO 0x110 (memcpy mode) tw686x :07:00.0: enabling device (0140 -> 0142) tw686x :07:00.0: dma0: unable to allocate P-buffer tw686x :07:00.0: can't register video tw686x: probe of :07:00.0 failed with error -12 -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure
Hi Krzysztof, On 10 May 2017 at 06:51, Krzysztof Hałasawrote: > Signed-off-by: Krzysztof Hałasa > > diff --git a/drivers/media/pci/tw686x/tw686x-video.c > b/drivers/media/pci/tw686x/tw686x-video.c > index c3fafa9..d637f47 100644 > --- a/drivers/media/pci/tw686x/tw686x-video.c > +++ b/drivers/media/pci/tw686x/tw686x-video.c > @@ -1190,6 +1190,13 @@ int tw686x_video_init(struct tw686x_dev *dev) > return err; > } > > + /* Initialize vc->dev and vc->ch for the error path first */ > + for (ch = 0; ch < max_channels(dev); ch++) { > + struct tw686x_video_channel *vc = >video_channels[ch]; > + vc->dev = dev; > + vc->ch = ch; > + } > + I'm not sure where is the oops this commit fixes, care to explain it to me? > for (ch = 0; ch < max_channels(dev); ch++) { > struct tw686x_video_channel *vc = >video_channels[ch]; > struct video_device *vdev; > @@ -1198,9 +1205,6 @@ int tw686x_video_init(struct tw686x_dev *dev) > spin_lock_init(>qlock); > INIT_LIST_HEAD(>vidq_queued); > > - vc->dev = dev; > - vc->ch = ch; > - > /* default settings */ > err = tw686x_set_standard(vc, V4L2_STD_NTSC); > if (err) Thanks, -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
[PATCH] TW686x: Fix OOPS on buffer alloc failure
Signed-off-by: Krzysztof Hałasadiff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c index c3fafa9..d637f47 100644 --- a/drivers/media/pci/tw686x/tw686x-video.c +++ b/drivers/media/pci/tw686x/tw686x-video.c @@ -1190,6 +1190,13 @@ int tw686x_video_init(struct tw686x_dev *dev) return err; } + /* Initialize vc->dev and vc->ch for the error path first */ + for (ch = 0; ch < max_channels(dev); ch++) { + struct tw686x_video_channel *vc = >video_channels[ch]; + vc->dev = dev; + vc->ch = ch; + } + for (ch = 0; ch < max_channels(dev); ch++) { struct tw686x_video_channel *vc = >video_channels[ch]; struct video_device *vdev; @@ -1198,9 +1205,6 @@ int tw686x_video_init(struct tw686x_dev *dev) spin_lock_init(>qlock); INIT_LIST_HEAD(>vidq_queued); - vc->dev = dev; - vc->ch = ch; - /* default settings */ err = tw686x_set_standard(vc, V4L2_STD_NTSC); if (err)