Date: Sat, 6 Nov 2021 10:43:59 -0600 From: Scott Long <scottl@samsco.org> To: Andriy Gapon <avg@freebsd.org> Cc: scsi@freebsd.org, hackers@freebsd.org Subject: Re: cam_periph_(un)mapmem issues with XPT_MMC_IO Message-ID: <DB7A03F9-C184-487F-86F9-64088D403A13@samsco.org> In-Reply-To: <62ee1f87-6240-708b-b324-49c87e0efca8@FreeBSD.org> References: <62ee1f87-6240-708b-b324-49c87e0efca8@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Andriy, I agree with your analysis. My opinion is that your second suggestion is pr= eferable, the mmc module should gets its own custom handlers. Thanks, Scott > On Nov 6, 2021, at 10:33 AM, Andriy Gapon <avg@freebsd.org> wrote: >=20 > =EF=BB=BF > I think that I see a few issues with cam_periph_(un)mapmem handling of XPT= _MMC_IO ccbs. >=20 > First, I think that this piece of code sets the length incorrectly: > data_ptrs[0] =3D (unsigned char **)&ccb->mmcio.cmd.data; > lengths[0] =3D sizeof(struct mmc_data *); > I think that it should be sizeof(struct mmc_data) as it seems that we want= to map the whole structure into the kernel memory. >=20 > Then, I think that this code is not safe / correct: > data_ptrs[1] =3D (unsigned char **)&ccb->mmcio.cmd.data->da= ta; > lengths[1] =3D ccb->mmcio.cmd.data->len; > I believe that the code accesses internals of ccb->mmcio.cmd.data via a us= erland pointer as that object is not mapped into the kernel address space ye= t and the pointer is not adjusted yet. >=20 > Finally, I think that there is a problem with unmapping of those two data b= uffers. In all other cases where cam_periph_unmapmem() works on multiple me= mory locations (numbufs > 2), those locations are independent of each other.= > But for XPT_MMC_IO one buffer contains a pointer to other buffer, so there= is a dependency between them. >=20 > It seems that there is an access to mmcio.cmd.data object via its kernel p= ointer after the object is unmapped from the kernel space because it comes b= efore mmcio.cmd.data->data in the array. >=20 > Running a command like > # camcontrol mmcsdcmd 2:0:0 -c 8 -a 0 -f 0x35 -l 512 >=20 > I get the following crash (on arm): > panic: vm_fault_lookup: fault on nofault entry, addr: 0xde367000 > cpuid =3D 2 > time =3D 1636189704 > KDB: stack backtrace: > db_trace_self() at db_trace_self > db_trace_self_wrapper() at db_trace_self_wrapper+0x30 > vpanic() at vpanic+0x17c > doadump() at doadump > vm_fault() at vm_fault+0x17b0 > vm_fault_trap() at vm_fault_trap+0x78 > abort_handler() at abort_handler+0x3c8 > exception_exit() at exception_exit > cam_periph_unmapmem() at cam_periph_unmapmem+0x2e4 > passsendccb() at passsendccb+0x194 > passdoioctl() at passdoioctl+0xcc > passioctl() at passioctl+0x28 > devfs_ioctl() at devfs_ioctl+0xcc > vn_ioctl() at vn_ioctl+0x12c > devfs_ioctl_f() at devfs_ioctl_f+0x2c > kern_ioctl() at kern_ioctl+0x318 > sys_ioctl() at sys_ioctl+0x108 >=20 > Unfortunately I do not have a crash dump, only a serial console output. > As far as I can tell cam_periph_unmapmem+0x2e4 corresponds to the assignme= nt in the following code: > /* Set the user's pointer back to the original value */ > *data_ptrs[i] =3D mapinfo->orig[i]; >=20 > As a quick hack I tried to reverse the order of iteration and it seems to h= ave fixed the crash. >=20 > In general, it seems that cam_periph_(un)mapmem is not good for the MMC ca= se because of the dependency between buffers. Perhaps the code could be ext= ended to handle dependent buffers. Or maybe MMC should get its own special r= outines for mapping and unmapping the buffers. >=20 > Warner and Alexander, I Bcc-ed you for r320844 / > a94a63f0a6bc1 and r345656 / b059686a71c89. One added XPT_MMC_IO to cam_pe= riph_mapmem and the other added XPT_MMC_IO to cam_periph_unmapmem along with= multitude of other changes. >=20 > --=20 > Andriy Gapon >=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?DB7A03F9-C184-487F-86F9-64088D403A13>