Date: Fri, 15 May 2009 13:38:53 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Peter Holm <pho@freebsd.org> Cc: svn-src-head@freebsd.org, Ed Schouten <ed@80386.nl>, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r192094 - head/sys/kern Message-ID: <20090515103853.GE1927@deviant.kiev.zoral.com.ua> In-Reply-To: <20090515094852.GC1927@deviant.kiev.zoral.com.ua> References: <200905141054.n4EAsvp1088977@svn.freebsd.org> <20090515070239.GQ58540@hoeg.nl> <20090515080613.GA27593@x2.osted.lan> <20090515094852.GC1927@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--G6nVm6DDWH/FONJq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Fri, May 15, 2009 at 12:48:52PM +0300, Kostik Belousov wrote:
> On Fri, May 15, 2009 at 10:06:13AM +0200, Peter Holm wrote:
> > On Fri, May 15, 2009 at 09:02:39AM +0200, Ed Schouten wrote:
> > > Hi Kostik,
> > >=20
> > > * Konstantin Belousov <kib@FreeBSD.org> wrote:
> > > > Log:
> > > > Do not advance req->oldidx when sysctl_old_user returning an
> > > > error due to copyout failure or short buffer.
> > > > =20
> > > > The later breaks the usermode iterators of the sysctl results tha=
t pack
> > > > arbitrary number of variable-sized structures. Iterator expects t=
hat
> > > > kernel filled exactly oldlen bytes, and tries to interpret half-f=
illed
> > > > or garbage structure at the end of the buffer. In particular,
> > > > kinfo_getfile(3) segfaulted.
> > > > =20
> > > > Reported and tested by: pho
> > > > MFC after: 3 weeks
> > >=20
> > > Is it possible that this change introduces a regression? Right now
> > > `pstat -t' gets stuck in an infinite loop. I've added the following
> > > printf:
> > >=20
> > > | Index: pstat.c
> > > | =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > > | --- pstat.c (revision 192128)
> > > | +++ pstat.c (working copy)
> > > | @@ -263,6 +263,7 @@
> > > | if (errno !=3D ENOMEM)
> > > | err(1, "sysctlbyname()");
> > > | len *=3D 2;
> > > | + printf("Going to %zu\n", len);
> > > | if ((xttys =3D realloc(xttys, len)) =3D=3D NULL)
> > > | err(1, "realloc()");
> > > | }
> > >=20
> > > pstat on -CURRENT prints:
> > >=20
> > > | LINE INQ CAN LIN LOW OUTQ USE LOW COL SESS PGID S=
TATE
> > > | Going to 0
> > > | Going to 0
> > > | Going to 0
> > > | ...
> > >=20
> > > If I use the same patch on RELENG_6, I get the expected result:
> > >=20
> > > | LINE RAW CAN OUT IHIWT ILOWT OHWT LWT COL STATE SESS =
PGID DISC
> > > | Going to 272
> > > | Going to 544
> > > | Going to 1088
> > > | Going to 2176
> > > | Going to 4352
> > > | Going to 8704
> > > | sysmouse 0 0 0 0 0 0 0 0 - 0 =
0 term
> > > | ...
> > >=20
> > > So the problem is that sysctl overwrites the len argument with 0, even
> > > if it returns back to userspace with ENOMEM.
> > >=20
> > > I see we have two changes in sysctl. In theory it could also be relat=
ed
> > > to jhb@'s changes to sysctl locking, but I suspect it's less likely.
> > >=20
> >=20
> > I can confirm that it is r192094 that triggers the loop.
>=20
> Yes, this is what I mean when talked about a breakage.
>=20
> Below is the reversal of r192094 + the change to keep the old, ugly
> behaviour of sysctl kern.proc.filedesc to return 0 on ENOMEM, but with
> oldlen chopped at the end of the last completely written struct kern_info
> instead of the middle of partially-written one.
>=20
> Peter, could you, please, retest ?
Err, the patch.
diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index f29b0eb..e0008e6 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -2883,6 +2883,7 @@ sysctl_kern_proc_filedesc(SYSCTL_HANDLER_ARGS)
struct proc *p;
struct tty *tp;
int vfslocked;
+ size_t oldidx;
=20
name =3D (int *)arg1;
if ((p =3D pfind((pid_t)name[0])) =3D=3D NULL)
@@ -3061,14 +3062,26 @@ sysctl_kern_proc_filedesc(SYSCTL_HANDLER_ARGS)
strlen(kif->kf_path) + 1;
kif->kf_structsize =3D roundup(kif->kf_structsize,
sizeof(uint64_t));
+ oldidx =3D req->oldidx;
error =3D SYSCTL_OUT(req, kif, kif->kf_structsize);
- if (error)
+ if (error) {
+ if (error =3D=3D ENOMEM) {
+ /*
+ * The hack to keep the ABI of sysctl
+ * kern.proc.filedesc intact, but not
+ * to account a partially copied
+ * kinfo_file into the oldidx.
+ */
+ req->oldidx =3D oldidx;
+ error =3D 0;
+ }
break;
+ }
}
FILEDESC_SUNLOCK(fdp);
fddrop(fdp);
free(kif, M_TEMP);
- return (0);
+ return (error);
}
=20
static SYSCTL_NODE(_kern_proc, KERN_PROC_FILEDESC, filedesc, CTLFLAG_RD,
diff --git a/sys/kern/kern_sysctl.c b/sys/kern/kern_sysctl.c
index bf539be..0a8a096 100644
--- a/sys/kern/kern_sysctl.c
+++ b/sys/kern/kern_sysctl.c
@@ -1223,9 +1223,9 @@ sysctl_old_kernel(struct sysctl_req *req, const void =
*p, size_t l)
if (i > 0)
bcopy(p, (char *)req->oldptr + req->oldidx, i);
}
+ req->oldidx +=3D l;
if (req->oldptr && i !=3D l)
return (ENOMEM);
- req->oldidx +=3D l;
return (0);
}
=20
@@ -1322,10 +1322,9 @@ sysctl_old_user(struct sysctl_req *req, const void *=
p, size_t l)
size_t i, len, origidx;
=20
origidx =3D req->oldidx;
- if (req->oldptr =3D=3D NULL) {
- req->oldidx +=3D l;
+ req->oldidx +=3D l;
+ if (req->oldptr =3D=3D NULL)
return (0);
- }
/*
* If we have not wired the user supplied buffer and we are currently
* holding locks, drop a witness warning, as it's possible that
@@ -1347,7 +1346,6 @@ sysctl_old_user(struct sysctl_req *req, const void *p=
, size_t l)
return (error);
if (i < l)
return (ENOMEM);
- req->oldidx +=3D l;
return (0);
}
=20
--G6nVm6DDWH/FONJq
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (FreeBSD)
iEYEARECAAYFAkoNRjwACgkQC3+MBN1Mb4iNVwCgxEsXB7aKNdZq+YnSK30UT0C6
24AAn1IaZdSk7cbb5fn9TUp+N2WIhqnt
=vaye
-----END PGP SIGNATURE-----
--G6nVm6DDWH/FONJq--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090515103853.GE1927>