Date: Tue, 17 Jul 2007 15:45:54 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Eric Anholt <anholt@FreeBSD.org> Cc: x11@FreeBSD.org Subject: Re: LOR when running google-earth Message-ID: <20070717124554.GB2200@deviant.kiev.zoral.com.ua> In-Reply-To: <469CAC37.1080205@micom.mng.net> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070717124554.GB2200>