On 2/28/20 11:46 PM, Antti Seppälä wrote: > On Fri, 28 Feb 2020 at 19:59, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: >> >> On Fri, Feb 28, 2020 at 08:14:35AM -0800, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Feb 27, 2020 at 8:28 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote: >>>> >>>> On 2/27/20 2:27 PM, Guenter Roeck wrote: >>>>> On 2/27/20 2:06 PM, Doug Anderson wrote: >>>> [ ... ] >>>>>>> - if (urb->num_sgs || urb->sg || >>>>>>> - urb->transfer_buffer_length == 0 || >>>>>>> + if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 || >>>>>>> + (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) || >>>>>>> !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) >>>>>> >>>>>> Maybe I'm misunderstanding things, but it feels like we need something >>>>>> more here. Specifically I'm worried about the fact when the transfer >>>>>> buffer is already aligned but the length is not a multiple of the >>>>>> endpoint's maximum transfer size. You need to handle that, right? >>>>>> AKA something like this (untested): >>>>>> >>>>>> /* Simple case of not having to allocate a bounce buffer */ >>>>>> if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 || >>>>>> (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) >>>>>> return 0; >>>>>> >>>>>> /* Can also avoid bounce buffer if alignment and size are good */ >>>>>> maxp = usb_endpoint_maxp(&ep->desc); >>>>>> if (maxp == urb->transfer_buffer_length && >>>>> >>>>> No, transfer_buffer_length would have to be a multiple of maxp. There >>>>> are many situations where roundup(transfer_buffer_length, maxp) != >>>>> transfer_buffer_length. I agree, this would be the prudent approach >>>>> (and it was my original implementation), but then it didn't seem to >>>>> cause trouble so far, and I was hesitant to add it in because it results >>>>> in creating temporary buffers for almost every receive operation. >>>>> I'd like to get some test feedback from Boris - if the current code >>>>> causes crashes with his use case, we'll know that it is needed. >>>>> Otherwise, we'll have to decide if the current approach (with fewer >>>>> copies) is worth the risk, or if we want to play save and always >>>>> copy if roundup(transfer_buffer_length, maxp) != transfer_buffer_length. >>>>> >>>> >>>> Thinking more about this, the situation is actually much worse: >>>> In Boris' testing, he found inbound transactions requested by usb >>>> storage code with a requested transfer size of 13 bytes ... with >>>> URB_NO_TRANSFER_DMA_MAP set. This means the requesting code has >>>> provided a DMA ready buffer, transfer_buffer isn't even used, >>>> and we can not reallocate it. In this situation we can just hope >>>> that the chip (and the connected USB device) don't send more data >>>> than requested. >>>> >>>> With that in mind, I think we should stick with the current >>>> scheme (ie only allocate a new buffer if the provided buffer is >>>> unaligned) unless Boris comes back and tells us that it doesn't >>>> work. >>> >>> I dunno. I'd rather see correctness over performance. Certainly we'd >>> only need to do the extra bounce buffer for input buffers at least. >>> >>> Although I don't love the idea, is this something where we want to >>> introduce a config option (either runtime or through KConfig), >>> something like: >>> >>> CONFIG_DWC2_FAST_AND_LOOSE - Avoid bounce buffers and thus run faster >>> at the risk of a bad USB device being able to clobber some of your >>> memory. Only do this if you really care about speed and have some >>> trust in the USB devices connected to your system. >>> >> >> I understand your point. Unfortunately that would only work if the driver >> doesn't set URB_NO_TRANSFER_DMA_MAP. >> >> $ git grep "=.*URB_NO_TRANSFER_DMA_MAP" | wc >> 115 498 10104 >> >> isn't exactly reassuring - a quick checks suggests that almost 50% >> of USB drivers set this flag. >> >> So all we'd really accomplish is to give people a false sense of >> security. >> >> In this context, I did play around with configuring the real receive >> buffer size (ie in my reproducer 1522 instead of 1536). If I do that, >> reading the HCTSIZ register after the transfer reports 0x7fff2 >> (or -14 = 1522-1536 if I treat the value as signed) as actual transfer >> size. Maybe that would be an option, if properly handled, but who knows >> what the IP actually does in this case, and what it does on other >> implementations (not rk3288). >> >> Guenter > > Hi Guenter. > > I decided to give your patch-set a spin on my Lantiq device and I'm > sorry to report that I'm now experiencing a complete crash of the > kernel due to unaligned access if I try to do usb transfers: > > Unhandled kernel unaligned access[#1]: > CPU: 1 PID: 3149 Comm: sh Not tainted 5.6-rc3 #0 > task: 87db2580 task.stack: 868c6000 > $ 0 : 00000000 00000001 00000000 81125088 > $ 4 : 8064ffc4 00000001 00000001 2a624a29 > $ 8 : 00000c43 00000c42 80010770 00000000 > $12 : 7f801be8 77fac2a0 00000022 00000000 > $16 : 87c02300 014000c0 87d1ddc0 00000000 > $20 : 87db2580 00000000 00000000 00000000 > $24 : 00000000 77f5fbcc > $28 : 868c6000 868c7e00 00000000 800347b0 > Hi : 0000001b > Lo : 0000005b > epc : 8012d278 kmem_cache_alloc+0x128/0x17c > ra : 800347b0 copy_process.part.94+0xd8/0x1690 > Status: 1100c303 KERNEL EXL IE > Cause : 00800010 (ExcCode 04) > BadVA : 2a624a29 > PrId : 00019556 (MIPS 34Kc) > Modules linked in: ath9k ath9k_common option ath9k_hw ath10k_pci > ath10k_core ath usb_wwan qmi_wwan pppoe nf_conntrack_ipv6 mac80211 > iptable_nat ipt_REJECT ipt_MASQUERADE cfg80211 xt_time xt_tcpudp > xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit xt_conntrack > xt_comment xt_TCPMSS xt_REDIRECT xt_LOG xt_FLOWOFFLOAD usbserial > usbnet ums_usbat ums_sddr55 ums_sddr09 ums_karma ums_jumpshot > ums_isd200 ums_freecom ums_datafab ums_cypress ums_alauda pppox > ppp_async owl_loader nf_reject_ipv4 nf_nat_redirect > nf_nat_masquerade_ipv4 nf_conntrack_ipv4 nf_nat_ipv4 nf_nat > nf_log_ipv4 nf_flow_table_hw nf_flow_table nf_defrag_ipv6 > nf_defrag_ipv4 nf_conntrack_rtcache nf_conntrack ltq_deu_vr9 > iptable_mangle iptable_filter ip_tables crc_ccitt compat cdc_wdm > drv_dsl_cpe_api drv_mei_cpe nf_log_ipv6 nf_log_common > ip6table_mangle ip6table_filter ip6_tables ip6t_REJECT x_tables > nf_reject_ipv6 pppoatm ppp_generic slhc br2684 atm drv_ifxos > usb_storage dwc2 sd_mod scsi_mod gpio_button_hotplug mii > Process sh (pid: 3149, threadinfo=868c6000, task=87db2580, tls=77fadefc) > Stack : 80657098 00000100 77fa6db0 00000000 00000012 00000012 ffffffff 800347b0 > 80650000 8013906c 87c02c00 8012d9fc 00000000 00000000 00000020 807b0000 > 879f1148 00000001 868c7e98 804f6018 00000000 801391a8 868c7ef0 80153258 > 00000000 00000001 864f62a0 00000020 864f62b8 80044818 00000400 8015fe94 > 00000003 00000005 00000012 00000000 7f801be0 00430871 00000000 77fac000 > ... > Call Trace: > [<8012d278>] kmem_cache_alloc+0x128/0x17c > [<800347b0>] copy_process.part.94+0xd8/0x1690 > [<80035f00>] _do_fork+0xe4/0x3bc > [<80036238>] sys_fork+0x24/0x30 > [<8001c438>] syscall_common+0x34/0x58 > Code: 00000000 8e020014 00e23821 <8ce20000> 10000009 cc400000 > 1040ffbd 00000000 8e060010 > > ---[ end trace 3d8df00f1a0d123c ]--- > > > Don't be fooled by the Call Trace, the stack unwinder is most likely > totally confused due to memory corruption. > > The culprit is the first patch in the series that does not align DMA > carefully enough. Apparently these big endian MIPS devices are very > picky on how that is supposed to be handled. > Sigh. It would have been too simple. Too bad I can't test myself. I'd like to know if this is because URB_NO_TRANSFER_DMA_MAP is set on a transfer, or because the beginning of the buffer indeed needs to be aligned to the DMA cache line size on that system. In the latter case, the question is why the alignment to DWC2_USB_DMA_ALIGN (=4) works. In the former case, question would be why the realignment does any good in the first place. Any chance you can add some test code to help figuring out what exactly goes wrong ? Thanks, Guenter > Couple of years ago mips people even contemplated on adding warn_on to > kernel to yell at driver authors who do not align their DMA properly. > [1.] > That patch explanation actually served as an inspiration for commit > 56406e017a88 in the first place. > > [1.] https://www.linux-mips.org/archives/linux-mips/2016-11/msg00267.html >
