From nobody Sat Nov 06 16:43:59 2021 X-Original-To: scsi@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 25713184ED7C; Sat, 6 Nov 2021 16:44:05 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from wout5-smtp.messagingengine.com (wout5-smtp.messagingengine.com [64.147.123.21]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4HmjrN6rc1z3J66; Sat, 6 Nov 2021 16:44:04 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.west.internal (Postfix) with ESMTP id 618B83200B23; Sat, 6 Nov 2021 12:44:03 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Sat, 06 Nov 2021 12:44:03 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsco.org; h= content-type:content-transfer-encoding:from:mime-version:subject :date:message-id:references:cc:in-reply-to:to; s=fm1; bh=2X0xqdw OCXEB1izjZRi/kWo+GZH9IA4aaX4oMi9D550=; b=Tk71eLvRBxqYLeiAA1mZSHV FnXb9ouWH7ismoZvFRT4RCTvbf8nYo6fa+4NsZOdqjM4ZLO6U9XtDDJKP5HJZ5dK 8cP4tjmJjghSkR+W3iXQv+4QtWkSG0F27nFdYdrCLGEZahq3IIEEgJtZuTDTQiO/ o2Fr/KiS4dxGZySDPD5ftW4puZFJUW5xOHwsIPbXALRRbyow3wZqNWwMtfJIOJLa STZQFezDBurgH5G5zs8IarSr5c8VgTyrX49eA37F1T13pdbKKM0/afZyvw3wnFud G2yIhYGQFsPfduzKtYW8MmSrnQrMsY24csFC0GkHD0iVJ7q2ERZsboMM8nQtUvA= = DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=2X0xqdwOCXEB1izjZRi/kWo+GZH9IA4aaX4oMi9D5 50=; b=Wm0mjMFxinOvq1lX9aLxuYnK+UsXsmYhGW9Vc7Biu+vxAKNIj2AyxuQwm la75fHDlW2WFwKB3i5b0B1ujvgMXxpfloTvV81ZlVlo/Po4weuB5pslMD0PPfro3 jAYMizXUdAyRr3sJsBYFYUY0mc60l/PJPQwKkuoeAIIM9HR77SzshUdYqPbpsK7X Q+rihGzhtbO+wqP/LBRJxPlJ3DVBKHnCVPC4vkStuOQesoXTXGWelz6jskqPC1xy EzNnHzsTZx58BJB1rUz+ql1tPzSsdAdDAZUALzh4SlQVL6aq8oxicO1zo8C9BBET vFPkb8NJFfxZFrI6J0f6Jic+W4xdA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddrtdekgdeltdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecunecujfgurheptgfghfggufffkfhfjgfvofesthhqmh dthhdtjeenucfhrhhomhepufgtohhtthcunfhonhhguceoshgtohhtthhlsehsrghmshgt ohdrohhrgheqnecuggftrfgrthhtvghrnhepheeuueetvefgieetuedvfeejhfeifefgte eigfejueduvdetkeelgeehfeevffeinecuffhomhgrihhnpegtmhgurdgurghtrgenucev lhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehstghothhtlh esshgrmhhstghordhorhhg X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 6 Nov 2021 12:44:02 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Scott Long List-Id: SCSI subsystem List-Archive: https://lists.freebsd.org/archives/freebsd-scsi List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-scsi@freebsd.org X-BeenThere: freebsd-scsi@freebsd.org Mime-Version: 1.0 (1.0) Subject: Re: cam_periph_(un)mapmem issues with XPT_MMC_IO Date: Sat, 6 Nov 2021 10:43:59 -0600 Message-Id: References: <62ee1f87-6240-708b-b324-49c87e0efca8@FreeBSD.org> Cc: scsi@freebsd.org, hackers@freebsd.org In-Reply-To: <62ee1f87-6240-708b-b324-49c87e0efca8@FreeBSD.org> To: Andriy Gapon X-Mailer: iPhone Mail (19A346) X-Rspamd-Queue-Id: 4HmjrN6rc1z3J66 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N 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 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