From owner-freebsd-x11@FreeBSD.ORG Tue Jul 17 12:46:20 2007 Return-Path: X-Original-To: x11@FreeBSD.org Delivered-To: freebsd-x11@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id BB4CA16A402; Tue, 17 Jul 2007 12:46:20 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from relay01.kiev.sovam.com (relay01.kiev.sovam.com [62.64.120.200]) by mx1.freebsd.org (Postfix) with ESMTP id 4C91313C491; Tue, 17 Jul 2007 12:46:20 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from [89.162.146.170] (helo=skuns.kiev.zoral.com.ua) by relay01.kiev.sovam.com with esmtps (TLSv1:AES256-SHA:256) (Exim 4.67) (envelope-from ) id 1IAmRL-00010o-WE; Tue, 17 Jul 2007 15:46:13 +0300 Received: from deviant.kiev.zoral.com.ua (root@[10.1.1.148]) by skuns.kiev.zoral.com.ua (8.14.1/8.14.1) with ESMTP id l6HCjsoX057214 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 17 Jul 2007 15:45:54 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.1/8.14.1) with ESMTP id l6HCjsa9032744; Tue, 17 Jul 2007 15:45:54 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.1/8.14.1/Submit) id l6HCjsqu032743; Tue, 17 Jul 2007 15:45:54 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 17 Jul 2007 15:45:54 +0300 From: Kostik Belousov To: Eric Anholt Message-ID: <20070717124554.GB2200@deviant.kiev.zoral.com.ua> References: <469B17BB.9050305@micom.mng.net> <20070716105316.GT2200@deviant.kiev.zoral.com.ua> <469C240B.4060201@micom.mng.net> <20070717042309.GY2200@deviant.kiev.zoral.com.ua> <469C4CCC.5000102@micom.mng.net> <20070717112639.GA2200@deviant.kiev.zoral.com.ua> <469CAC37.1080205@micom.mng.net> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="dv+7Y1Jmnoh5Cnne" Content-Disposition: inline In-Reply-To: <469CAC37.1080205@micom.mng.net> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: ClamAV version 0.90.3, clamav-milter version 0.90.3 on skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-1.4 required=5.0 tests=ALL_TRUSTED autolearn=failed version=3.2.1 X-Spam-Checker-Version: SpamAssassin 3.2.1 (2007-05-02) on skuns.kiev.zoral.com.ua X-Scanner-Signature: 20fd4dca2628540941c70b2970c1b995 X-DrWeb-checked: yes X-SpamTest-Envelope-From: kostikbel@gmail.com X-SpamTest-Group-ID: 00000000 X-SpamTest-Header: Not Detected X-SpamTest-Info: Profiles 1232 [July 17 2007] X-SpamTest-Info: helo_type=3 X-SpamTest-Method: none X-SpamTest-Rate: 0 X-SpamTest-Status: Not detected X-SpamTest-Status-Extended: not_detected X-SpamTest-Version: SMTP-Filter Version 3.0.0 [0255], KAS30/Release Cc: x11@FreeBSD.org Subject: Re: LOR when running google-earth X-BeenThere: freebsd-x11@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: X11 on FreeBSD -- maintaining and support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jul 2007 12:46:20 -0000 --dv+7Y1Jmnoh5Cnne Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 17, 2007 at 07:47:03PM +0800, Ganbold wrote: > Kostik Belousov wrote: > >On Tue, Jul 17, 2007 at 12:59:56PM +0800, Ganbold wrote: > > =20 > >>Thanks for the patch, Konstantin. > >>It seems like LOR is gone after the patch. > >>However I got following when I tried to switch virtual desktops under= =20 > >>beryl: > >> > >>error: [drm:pid1:i915_emit_box] *ERROR* Bad box 64485,100..0,774 > >>error: [drm:pid1:i915_cmdbuffer] *ERROR* i915_dispatch_cmdbuffer failed > >>error: [drm:pid1:i915_emit_box] *ERROR* Bad box 64485,100..0,774 > >>error: [drm:pid1:i915_cmdbuffer] *ERROR* i915_dispatch_cmdbuffer failed > >>error: [drm:pid1157:drm_close] *ERROR* can't find authenticator > >> > >>On shell where I'm running googleearth it shows: > >> > >>DRM_I830_CMDBUFFER: -22 > >> > >>and googleearth crashes :( > >> > >>Maybe above problems are related to beryl/emerald. > >> > >> =20 > >Ganbold, > >does the problems you described show only with patch applied ? Or, they = are > >reproducable without patch, on the stock kernel ? > > =20 > Konstantin, >=20 > I just tested stock i915 module. googleearth crashes with same message: >=20 > error: [drm:pid1:i915_emit_box] *ERROR* Bad box 64485,100..0,774 > error: [drm:pid1:i915_cmdbuffer] *ERROR* i915_dispatch_cmdbuffer failed >=20 > DRM_I830_CMDBUFFER: -22 >=20 > So my guess was correct. >=20 > thanks, >=20 > Ganbold Eric, could you, please, review some further change to the i915 drm driver. The LOR reported by Ganbold happens when useracc() checks the range of the user address space in the dev/drm/i915_dma.c, using the DRM_VERIFYAREA_READ= () macro. It calls useracc(), that locks sx lock for user map, while holding drm mutex. lock order reversal: (sleepable after non-sleepable) 1st 0xc3c06cd8 drm device (drm device) @=20 /usr/src/sys/modules/drm/drm/../../../dev/drm/drm_drv.c:907 2nd 0xc3e6ea3c user map (user map) @ /usr/src/sys/vm/vm_glue.c:182 KDB: stack backtrace: db_trace_self_wrapper(c08a3fa4,e6450a44,c066992e,c08a6485,c3e6ea3c,...)=20 at db_trace_self_wrapper+0x26 kdb_backtrace(c08a6485,c3e6ea3c,c08be517,c08be517,c08bdfed,...) at=20 kdb_backtrace+0x29 witness_checkorder(c3e6ea3c,9,c08bdfed,b6,c435a000,...) at=20 witness_checkorder+0x6de _sx_xlock(c3e6ea3c,0,c08bdfed,b6,e6450aa8,...) at _sx_xlock+0x7d _vm_map_lock_read(c3e6e9f8,c08bdfed,b6,c435a000,a60,...) at=20 _vm_map_lock_read+0x50 useracc(4ce1ede8,8,1,f5,c09ce1bc,...) at useracc+0x65 i915_cmdbuffer(c3bf3800,8018644b,c43814a0,3,c435a000,...) at=20 i915_cmdbuffer+0x10f drm_ioctl(c3bf3800,8018644b,c43814a0,3,c435a000,...) at drm_ioctl+0x357 giant_ioctl(c3bf3800,8018644b,c43814a0,3,c435a000,...) at giant_ioctl+0x56 devfs_ioctl_f(c4cdd1b0,8018644b,c43814a0,c4356700,c435a000,...) at=20 devfs_ioctl_f+0xc9 kern_ioctl(c435a000,9,8018644b,c43814a0,450c4c,...) at kern_ioctl+0x243 ioctl(c435a000,e6450cfc,e6450c80,c4022e4a,c435a000,...) at ioctl+0x134 linux_ioctl_drm(c435a000,e6450cfc,c4030c45,a2f,c435a000,...) at=20 linux_ioctl_drm+0x2f linux_ioctl(c435a000,e6450cfc,e6450cf8,e6450d1c,c4032dd0,...) at=20 linux_ioctl+0xca syscall(e6450d38) at syscall+0x2b3 Xint0x80_syscall() at Xint0x80_syscall+0x20 In fact, besides the LOR, there are two bigger problems: 1. the check is racy, since userspace may modity the address space after the check is done, but before the actual access. 2. useracc() does not page in the requested address, thus further LOR may happen while vm_fault() would handle the page in. The same LOR could occur due to use of DRM_COPY_FROM_USER_UNCHECKED(), if t= he page is swapped out. Patch below tries to fix the problem for i915 driver (and it looks like the= se are all consumers of the DRM_VERIFY_AREA_READ) by wiring the user pages with the help of vslock(). Also, I eliminated DRM_COPY_FROM_USER_UNCHECKED. The obvious drawbacks of the patch are: 1. It further changes the drm code, making the FreeBSD fork of it. 2. vslock() causes user map fragmentation by clipping the wiring map entry to the wired region. 3. The only vslock() user in the CVS tree is sysctl(). I am not sure whether adding another one is good. diff --git a/sys/dev/drm/i915_dma.c b/sys/dev/drm/i915_dma.c index dfaf334..c9dd799 100644 --- a/sys/dev/drm/i915_dma.c +++ b/sys/dev/drm/i915_dma.c @@ -367,20 +367,14 @@ static int i915_emit_cmds(drm_device_t * dev, int __u= ser * buffer, int dwords) for (i =3D 0; i < dwords;) { int cmd, sz; =20 - if (DRM_COPY_FROM_USER_UNCHECKED(&cmd, &buffer[i], sizeof(cmd))) { - - return DRM_ERR(EINVAL); - } + cmd =3D buffer[i]; if ((sz =3D validate_cmd(cmd)) =3D=3D 0 || i + sz > dwords) return DRM_ERR(EINVAL); =20 OUT_RING(cmd); =20 while (++i, --sz) { - if (DRM_COPY_FROM_USER_UNCHECKED(&cmd, &buffer[i], - sizeof(cmd))) { - return DRM_ERR(EINVAL); - } + cmd =3D buffer[i]; OUT_RING(cmd); } } @@ -401,10 +395,7 @@ static int i915_emit_box(drm_device_t * dev, drm_clip_rect_t box; RING_LOCALS; =20 - if (DRM_COPY_FROM_USER_UNCHECKED(&box, &boxes[i], sizeof(box))) { - return EFAULT; - } - + box =3D boxes[i]; if (box.y2 <=3D box.y1 || box.x2 <=3D box.x1 || box.y2 <=3D 0 || box.x2 <= =3D 0) { DRM_ERROR("Bad box %d,%d..%d,%d\n", box.x1, box.y1, box.x2, box.y2); @@ -604,6 +595,7 @@ static int i915_batchbuffer(DRM_IOCTL_ARGS) drm_i915_sarea_t *sarea_priv =3D (drm_i915_sarea_t *) dev_priv->sarea_priv; drm_i915_batchbuffer_t batch; + size_t cliplen; int ret; =20 if (!dev_priv->allow_batchbuffer) { @@ -619,14 +611,25 @@ static int i915_batchbuffer(DRM_IOCTL_ARGS) =20 LOCK_TEST_WITH_RETURN(dev, filp); =20 + DRM_UNLOCK(); + cliplen =3D batch.num_cliprects * sizeof(drm_clip_rect_t);=09 if (batch.num_cliprects && DRM_VERIFYAREA_READ(batch.cliprects, - batch.num_cliprects * - sizeof(drm_clip_rect_t))) + cliplen)) { + DRM_LOCK(); return DRM_ERR(EFAULT); - + } + ret =3D vslock(batch.cliprects, cliplen); + if (ret) { + DRM_ERROR("Fault wiring cliprects\n"); + DRM_LOCK(); + return DRM_ERR(EFAULT); + } + DRM_LOCK(); ret =3D i915_dispatch_batchbuffer(dev, &batch); - sarea_priv->last_dispatch =3D (int)hw_status[5]; + DRM_UNLOCK(); + vsunlock(batch.cliprects, cliplen); + DRM_LOCK(); return ret; } =20 @@ -638,6 +641,7 @@ static int i915_cmdbuffer(DRM_IOCTL_ARGS) drm_i915_sarea_t *sarea_priv =3D (drm_i915_sarea_t *) dev_priv->sarea_priv; drm_i915_cmdbuffer_t cmdbuf; + size_t cliplen; int ret; =20 DRM_COPY_FROM_USER_IOCTL(cmdbuf, (drm_i915_cmdbuffer_t __user *) data, @@ -648,22 +652,38 @@ static int i915_cmdbuffer(DRM_IOCTL_ARGS) =20 LOCK_TEST_WITH_RETURN(dev, filp); =20 + DRM_UNLOCK(); + cliplen =3D cmdbuf.num_cliprects * sizeof(drm_clip_rect_t); if (cmdbuf.num_cliprects && - DRM_VERIFYAREA_READ(cmdbuf.cliprects, - cmdbuf.num_cliprects * - sizeof(drm_clip_rect_t))) { + DRM_VERIFYAREA_READ(cmdbuf.cliprects, cliplen)) { DRM_ERROR("Fault accessing cliprects\n"); + DRM_LOCK(); return DRM_ERR(EFAULT); } - - ret =3D i915_dispatch_cmdbuffer(dev, &cmdbuf); + ret =3D vslock(cmdbuf.cliprects, cliplen); if (ret) { - DRM_ERROR("i915_dispatch_cmdbuffer failed\n"); - return ret; + DRM_ERROR("Fault wiring cliprects\n"); + DRM_LOCK(); + return DRM_ERR(EFAULT); } - - sarea_priv->last_dispatch =3D (int)hw_status[5]; - return 0; + ret =3D vslock(cmdbuf.buf, cmdbuf.sz); + if (ret) { + vsunlock(cmdbuf.cliprects, cliplen); + DRM_ERROR("Fault wiring cmds\n"); + DRM_LOCK(); + return DRM_ERR(EFAULT); + } + DRM_LOCK(); + ret =3D i915_dispatch_cmdbuffer(dev, &cmdbuf); + if (ret =3D=3D 0) + sarea_priv->last_dispatch =3D (int)hw_status[5]; + else + DRM_ERROR("i915_dispatch_cmdbuffer failed\n"); + DRM_UNLOCK(); + vsunlock(cmdbuf.buf, cmdbuf.sz); + vsunlock(cmdbuf.cliprects, cliplen); + DRM_LOCK(); + return (ret); } =20 static int i915_do_cleanup_pageflip(drm_device_t * dev) --dv+7Y1Jmnoh5Cnne Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (FreeBSD) iD8DBQFGnLoBC3+MBN1Mb4gRAnimAKCZggW1/EPwvNjGGGCIsMaT1AbHqACg6DXr DHc9Ke4fz2UG3hpEq0DiZCw= =lLhk -----END PGP SIGNATURE----- --dv+7Y1Jmnoh5Cnne--