svn commit: r192094 - head/sys/kern

Kostik Belousov kostikbel at gmail.com
Fri May 15 10:39:00 UTC 2009


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,
> > > 
> > > * Konstantin Belousov <kib at FreeBSD.org> wrote:
> > > > Log:
> > > >   Do not advance req->oldidx when sysctl_old_user returning an
> > > >   error due to copyout failure or short buffer.
> > > >   
> > > >   The later breaks the usermode iterators of the sysctl results that pack
> > > >   arbitrary number of variable-sized structures. Iterator expects that
> > > >   kernel filled exactly oldlen bytes, and tries to interpret half-filled
> > > >   or garbage structure at the end of the buffer. In particular,
> > > >   kinfo_getfile(3) segfaulted.
> > > >   
> > > >   Reported and tested by:	pho
> > > >   MFC after:	3 weeks
> > > 
> > > 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:
> > > 
> > > | Index: pstat.c
> > > | ===================================================================
> > > | --- pstat.c	(revision 192128)
> > > | +++ pstat.c	(working copy)
> > > | @@ -263,6 +263,7 @@
> > > |  		if (errno != ENOMEM)
> > > |  			err(1, "sysctlbyname()");
> > > |  		len *= 2;
> > > | +		printf("Going to %zu\n", len);
> > > |  		if ((xttys = realloc(xttys, len)) == NULL)
> > > |  			err(1, "realloc()");
> > > |  	}
> > > 
> > > pstat on -CURRENT prints:
> > > 
> > > |       LINE   INQ  CAN  LIN  LOW  OUTQ  USE  LOW   COL  SESS  PGID STATE
> > > | Going to 0
> > > | Going to 0
> > > | Going to 0
> > > | ...
> > > 
> > > If I use the same patch on RELENG_6, I get the expected result:
> > > 
> > > |      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
> > > | ...
> > > 
> > > So the problem is that sysctl overwrites the len argument with 0, even
> > > if it returns back to userspace with ENOMEM.
> > > 
> > > I see we have two changes in sysctl. In theory it could also be related
> > > to jhb@'s changes to sysctl locking, but I suspect it's less likely.
> > > 
> > 
> > I can confirm that it is r192094 that triggers the loop.
> 
> Yes, this is what I mean when talked about a breakage.
> 
> 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.
> 
> 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;
 
 	name = (int *)arg1;
 	if ((p = pfind((pid_t)name[0])) == NULL)
@@ -3061,14 +3062,26 @@ sysctl_kern_proc_filedesc(SYSCTL_HANDLER_ARGS)
 		    strlen(kif->kf_path) + 1;
 		kif->kf_structsize = roundup(kif->kf_structsize,
 		    sizeof(uint64_t));
+		oldidx = req->oldidx;
 		error = SYSCTL_OUT(req, kif, kif->kf_structsize);
-		if (error)
+		if (error) {
+			if (error == 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 = oldidx;
+				error = 0;
+			}
 			break;
+		}
 	}
 	FILEDESC_SUNLOCK(fdp);
 	fddrop(fdp);
 	free(kif, M_TEMP);
-	return (0);
+	return (error);
 }
 
 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 += l;
 	if (req->oldptr && i != l)
 		return (ENOMEM);
-	req->oldidx += l;
 	return (0);
 }
 
@@ -1322,10 +1322,9 @@ sysctl_old_user(struct sysctl_req *req, const void *p, size_t l)
 	size_t i, len, origidx;
 
 	origidx = req->oldidx;
-	if (req->oldptr == NULL) {
-		req->oldidx += l;
+	req->oldidx += l;
+	if (req->oldptr == 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 += l;
 	return (0);
 }
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20090515/55abb28f/attachment.pgp


More information about the svn-src-all mailing list