Bug 15977

This is a dvb driver issue, not a usb core issue.  The usb core is properly
warning you that this driver needs to be fixed to not use memory
off of the stack.

Created attachment 50882 [details]
Proposed fix

Hi Zdenek, 

can you test this patch ontop of 2.6.37 or 2.6.38-rc8?

Well - I've applied on currently a bit risky 2.6.39-pre-rc
(6c5103890057b1bb781b26b7aae38d33e4c517d8)

and driver doesn't work at all.

Attaching backtraces.

Created attachment 52092 [details]
Trace with patch applied

There is still same DMA buffer on stack.
And i2c seems to crash completely

Thanks. Sorry for late response. Bugzilla didn't send out email notifications again... 

The maintainer has a patch which somehow somewhere goes upstream somewhen... 

 http://git.linuxtv.org/pb/media_tree.git?a=commit;h=16b54de2d8b46e48c5c8bdf9b350eac04e8f6b46

That should probably work, since I hope they have the hardware.  :)

Does it?

Regards,
Flo

Is there anyone testing it?
It still broken.

The patch you point out in comment 5:

dib0700: firmware started successfully.
dvb-usb: found a 'Hauppauge Nova-TD Stick (52009)' in warm state.
dvb-usb: will pass the complete MPEG2 transport stream to the software demuxer.
DVB: registering new adapter (Hauppauge Nova-TD Stick (52009))
------------[ cut here ]------------
WARNING: at lib/dma-debug.c:867 check_for_stack+0xb3/0xf0()
Hardware name: 6464CTO
ehci_hcd 0000:00:1d.7: DMA-API: device driver maps memory fromstack [addr=ffff880138b13bd6]
Modules linked in: ir_lirc_codec dvb_usb_dib0700(+) lirc_dev dib7000p ir_sony_decoder dib7000m dib0070 ir_jvc_decoder dvb_usb ir_rc6_decoder dib8000 dvb_core ir_rc5_decoder ir_nec_decoder dib3000mc rc_core dibx000_common ip6_tables ebtable_nat ebtables iptable_mangle xt_tcpudp tun bridge ipv6 stp llc sunrpc bluetooth ipt_REJECT xt_physdev xt_state iptable_filter ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4 ip_tables x_tables snd_hda_codec_analog arc4 ecb crypto_blkcipher cryptomgr aead crypto_algapi iwl3945 iwl_legacy snd_hda_intel mac80211 snd_hda_codec snd_seq snd_seq_device psmouse serio_raw cfg80211 e1000e snd_pcm i2c_i801 iTCO_wdt thinkpad_acpi snd_timer iTCO_vendor_support snd_page_alloc wmi snd soundcore nvram evdev i915 dm_mirror dm_region_hash dm_log dm_mod drm_kms_helper drm i2c_algo_bit i2c_core autofs4 usbhid hid pcmcia sdhci_pci uhci_hcd sr_mod ehci_hcd sdhci yenta_socket mmc_core cdrom usbcore video backlight
Pid: 1185, comm: modprobe Not tainted 2.6.39-rc3-00236-g115ab9e #7
Call Trace:
 [<ffffffff810501df>] warn_slowpath_common+0x7f/0xc0
 [<ffffffff810502d6>] warn_slowpath_fmt+0x46/0x50
 [<ffffffff81496d69>] ? sub_preempt_count+0xa9/0xe0
 [<ffffffff812a3713>] check_for_stack+0xb3/0xf0
 [<ffffffff812a3acf>] debug_dma_map_page+0xff/0x150
 [<ffffffffa00baea2>] usb_hcd_map_urb_for_dma+0x522/0x590 [usbcore]
 [<ffffffff81496d69>] ? sub_preempt_count+0xa9/0xe0
 [<ffffffffa00bb055>] usb_hcd_submit_urb+0x145/0x7e0 [usbcore]
 [<ffffffff8108b463>] ? lockdep_init_map+0xb3/0x560
 [<ffffffffa00bc3e4>] usb_submit_urb+0xf4/0x390 [usbcore]
 [<ffffffffa00bd94c>] usb_start_wait_urb+0x6c/0x170 [usbcore]
 [<ffffffffa00bc7d5>] ? usb_init_urb+0x55/0xf0 [usbcore]
 [<ffffffffa00bdcd6>] usb_control_msg+0xe6/0x120 [usbcore]
 [<ffffffffa0571404>] ? dib0700_i2c_xfer+0x64/0x350 [dvb_usb_dib0700]
 [<ffffffffa0571404>] ? dib0700_i2c_xfer+0x64/0x350 [dvb_usb_dib0700]
 [<ffffffffa0571368>] dib0700_ctrl_rd+0x88/0xc0 [dvb_usb_dib0700]
 [<ffffffffa05714da>] dib0700_i2c_xfer+0x13a/0x350 [dvb_usb_dib0700]
 [<ffffffffa0044f3a>] i2c_transfer+0xaa/0x120 [i2c_core]
 [<ffffffffa055989e>] dib7000p_read_word+0x6e/0xd0 [dib7000p]
 [<ffffffffa055adf1>] dib7000p_identify+0x31/0x110 [dib7000p]
 [<ffffffffa055afcb>] dib7000p_i2c_enumeration+0xfb/0x310 [dib7000p]
 [<ffffffffa0571082>] ? dib0700_rc_urb_completion+0x12/0x140 [dvb_usb_dib0700]
 [<ffffffffa0573b6c>] stk7070pd_frontend_attach0+0xfc/0x1c0 [dvb_usb_dib0700]
 [<ffffffffa04ed46e>] dvb_usb_adapter_frontend_init+0x1e/0x110 [dvb_usb]
 [<ffffffffa04ec960>] dvb_usb_device_init+0x390/0x670 [dvb_usb]
 [<ffffffffa0571fca>] dib0700_probe+0x6a/0xe0 [dvb_usb_dib0700]
 [<ffffffffa00c10b6>] usb_probe_interface+0xf6/0x250 [usbcore]
 [<ffffffff81354c2c>] driver_probe_device+0x9c/0x2b0
 [<ffffffff81354eeb>] __driver_attach+0xab/0xb0
 [<ffffffff81354e40>] ? driver_probe_device+0x2b0/0x2b0
 [<ffffffff81354e40>] ? driver_probe_device+0x2b0/0x2b0
 [<ffffffff81353a24>] bus_for_each_dev+0x64/0xa0
 [<ffffffff8135484e>] driver_attach+0x1e/0x20
 [<ffffffff81354440>] bus_add_driver+0x1c0/0x2b0
 [<ffffffff81355466>] driver_register+0x76/0x140
 [<ffffffff81298028>] ? __raw_spin_lock_init+0x38/0x70
 [<ffffffffa00bfe75>] usb_register_driver+0xc5/0x1b0 [usbcore]
 [<ffffffffa058e000>] ? 0xffffffffa058dfff
 [<ffffffffa058e034>] dib0700_module_init+0x34/0x51 [dvb_usb_dib0700]
 [<ffffffff810001d2>] do_one_initcall+0x42/0x180
 [<ffffffff8109df6b>] sys_init_module+0xab/0x200
 [<ffffffff8149ac6b>] system_call_fastpath+0x16/0x1b
---[ end trace 5934ecdc00d47ffa ]---
DVB: registering adapter 0 frontend 0 (DiBcom 7000PC)...
DiB0070: successfully identified
dvb-usb: will pass the complete MPEG2 transport stream to the software demuxer.
DVB: registering new adapter (Hauppauge Nova-TD Stick (52009))

Somehow the maintainers email seems to be a bit ...slow going...   don't know. Sorry.

Usage of stack in dib0700_ctrl_rd()  from:
media/dvb/dvb-usb/dib0700_core.c seems to be the culprit here.

From the look upward in the stack - it looks like the stack came from:
dib7000p_read_word() in drivers/media/dvb/frontends/dip7000p.c which has:
'u8 wb[2]' and 'u8 rb[2]' located on the stack.

On the other hand this 'technique' is quite widespread over the whole
frontends subdir.

So this makes me curious - how it could work elsewhere ?

Maybe dib0700_i2c_xfer() implementation should use it's own kmalloced buffers and copy the result into 'stack' passed allocated message buffer ?

Or maybe even  usb_control_msg() should hide this trick ?

DMA on stack could theoretically work but is problematic due to coherency issues. 

The original reason the restriction was put in place is that the dma buffer has to be mapped to physical memory in order for the device and the cpu to be able to interact. On some archs, the stack is (or was?) apparently mapped virtually. [1]

A more relevant concern in this case is the issue of cache coherency. Changes done by the device to the memory in question have to be visible to the cpu. 
This is done by performing some operations on the cache (flushing to main memory before the dma and causing the memory to be read back in cache afterwards, I guess). [3]

If the memory is now laid out as such that the dma buffer shares a cacheline with another data structure on the stack, then the cacheline _may_ be read back _while the dma is in progress_ because the cpu or another cpu accesses that data structure, the whole cacheline is read back, leading to an inconsistent view of the memory. (and probably the dma framework is then not able to cause the cpu to re-read that cacheline after the dma has finished, because it is already loaded)

See also the respective mailingthreads on linux-media. [2]

Also, if you want some Documentation:
The buffer to usb_control_msg get's to be
a parameter to dma_map_single which is described as part of
the DMA API in Documentation/DMA-API.txt 


Using a seperately kmalloced buffer is sufficient. 

Using a preallocated memory buffer also get's rid of the kmalloc overhead. But is probably not necessary. If you use a preallocated buffer, you have to protect it against parallel access. AFAICS it should be sufficient to reuse the i2c_mutex from the dvb_usb_device. 

dib07000p is new in 2.6.39-rc... I wonder how your 2.6.36 regression can stem from that :) ... but yeah of course it's the same cause. Probably the driver switched over to the new code without you knowing. doesn't matter anyway, but that is why I overlooked it in my patch submissions till now. 
I still wait for a reply to my email[4] from Patrick.


Regards,
Flo

[4] https://lkml.org/lkml/2011/4/4/228 
[3] https://lkml.org/lkml/2011/3/22/182
[2] https://lkml.org/lkml/2011/3/21/282
[1] https://lkml.org/lkml/2011/3/22/339

(In reply to comment #9)
> 
> 
> DMA on stack could theoretically work but is problematic due to coherency
> issues. 
> 
> The original reason the restriction was put in place is that the dma buffer
> has
> to be mapped to physical memory in order for the device and the cpu to be
> able
> to interact. On some archs, the stack is (or was?) apparently mapped
> virtually.
> [1]
> 
> A more relevant concern in this case is the issue of cache coherency. Changes
> done by the device to the memory in question have to be visible to the cpu. 
> This is done by performing some operations on the cache (flushing to main
> memory before the dma and causing the memory to be read back in cache
> afterwards, I guess). [3]
> 

Well I assume you would hit even some troubles when some devices are not able to properly work with >4G and DMA - so some proper memory needs to be allocated as well.


> dib07000p is new in 2.6.39-rc... I wonder how your 2.6.36 regression can stem
> from that :) ... but yeah of course it's the same cause. Probably the driver
> switched over to the new code without you knowing. doesn't matter anyway, but
> that is why I overlooked it in my patch submissions till now. 
> I still wait for a reply to my email[4] from Patrick.

If this would be question for me - it's not a regression - it's persistent bug in the whole dvb-t subdir :)

Also 'I have a dream' of properly working suspend/resume with DVB-T in my computer.

(In reply to comment #10) 
> Well I assume you would hit even some troubles when some devices are not able
> to properly work with >4G and DMA - so some proper memory needs to be
> allocated
> as well.

kmalloc is said to work fine with GFP_KERNEL