From owner-svn-src-all@freebsd.org Wed Apr 19 04:10:02 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E5001D44F3E; Wed, 19 Apr 2017 04:10:02 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 919FE926; Wed, 19 Apr 2017 04:10:02 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id AF7A41A5E5E; Wed, 19 Apr 2017 14:09:59 +1000 (AEST) Date: Wed, 19 Apr 2017 14:09:58 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Alan Somers cc: Gleb Smirnoff , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r317061 - in head: libexec/rpc.rstatd sys/amd64/amd64 sys/amd64/include sys/arm/arm sys/arm/include sys/arm64/include sys/cddl/contrib/opensolaris/uts/common/fs/zfs sys/compat/linprocfs... In-Reply-To: Message-ID: <20170419130428.J956@besplex.bde.org> References: <201704171734.v3HHYlf5022945@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=KeqiiUQD c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=4wcEti7meZBUi9NaIXUA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Apr 2017 04:10:03 -0000 On Tue, 18 Apr 2017, Alan Somers wrote: > On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff wrote: Please trim quotes. [... 5 lines of quotes trimmed] >> Log: >> - Remove 'struct vmmeter' from 'struct pcpu', leaving only global vmmeter >> in place. To do per-cpu stats, convert all fields that previously were >> maintained in the vmmeters that sit in pcpus to counter(9). [... 21 lines of quotes trimmed] [... more than 50 lines of quotes trimmed] >> head/usr.bin/top/machine.c >> head/usr.bin/vmstat/vmstat.c The previous 2 lines turn out to be relevant. I missed the update to vmstat and checked a wrong version in my bug report described below I checked an updated top, but the change seemed too small to help. systat was not updated. > This change broke backwards compatibility with old top binaries. When > I use a kernel at version 317094 but a top from 14-April, I get the > error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate > memory". I get the same error when running top from an 11.0-RELEASE > jail. Can you please add backward compatibility shims? I sent a much longer (30 times longer) bug report to the author of the bug. Most or all statistics utilities that use vmmeter are broken. vmstat is too broken to even notice the bug -- it silently ignores the error, an has type errors which prevent other errors which it doesn't ignore. The result is silently truncating to 32 bits, like a quick fix would do intentionally. systat -v detects the errors but prints warning messages to the status line where they overwrite each other so make the problem look smaller than it is. The result is unsilently truncating to 32 bits. I've never seen any backwards compatibility shims added for sysctls. None should be needed, but there are few or no utilities other than sysctl(8) which use sysctl(3) in a portable way. Utilities tend to hard-code types and thus break when the types are expanded or shrunk. Especially broken ones like vmstat even ignore the error for their size being too small. There is no error for the size being too small, so callers must check if the size returned is the size supplied. vmstat is too broken to check this too. systat -v does check. Note that using the types in the system header is a form of hard-coding. This gives binaries that only work when run on the same system as the headers. Using the "any" type is best done using a smart wrapper or perhaps a smarter sysctl(3), and many utilities already have dumb wrappers. The one in vmstat() shows how not to do it: X static int X mysysctl(const char *name, void *oldp, size_t *oldlenp, X void *newp, size_t newlen) X { X int error; X X error = sysctlbyname(name, oldp, oldlenp, newp, newlen); X if (error != 0 && errno != ENOMEM) X xo_err(1, "sysctl(%s)", name); X return (error); X } ENOMEM here means that our variable is too small. It is not easy to expand correctly, but it is easy to check if it is small enough to cause a problem. Something like: // Zero-extend in case our variable is larger than the // system's. We only support unsigned variables. bzero(oldp, *oldlenp); // Don't use our garbage new* variables. All calls are // read-only since we are a stats utility, and this wrapper // doesn't support writing. if (sysctlbyname(name, oldp, oldlenp, NULL, 0) == 0) return (0) u_char buf[640 * 1024]; // should be enough for anyint size_t buflen; size_t off; buflen = sizeof(buf); error = sysctlbyname(name, buf, buflen, NULL, 0); if (error != 0) xo_err(1, "sysctl(%s)", name); for (off = *oldlenp; buf < bufsize; off++) { // Check for zero-extension. // This version only supports unsigned types. if (buf[off] != 0) { errno = EOVERFLOW; #ifdef CALLERS_PROBLEM // sysctl(3) should always have done this // too. That is, return EOVERFLOW instead // of ENOMEM when the result is too large // to fit, and don't return an error if // it does fit. return (-1); #else // Now a useful error message. xo_err(1, "sysctl(%s)", name); #endif } } Then expand sizes in the caller if necessary. u_int would work in most cases, and so would u_char for small and boolean variables. CALLERS_PROBLEM allows use of small types when only differences are needed, as for some cases in vmstat. uintmax_t would work in more cases. But it is still a form of hard-coding -- it will break when the system uintmax_t is increased to {128, 256, 512, ...} bits while the user uintmax_t remains uint64_t, and the extra bits are actually used. 64-bit types rarely overflow, but if the system type is larger then 64 bytes was not enough for anyint. The bugs in vmstat now go in the other direction. It now hard-codes uint64_t instead of u_int for the expanded types. Its missing error handling is now a more serious problem. When run on an old kernel, sysctl() returns 32 bits into the 64-bit variables and the error is not detected. Perhaps the variables are initially zero so this is not a problem, but the zeroing should be more intentional. It is very easy to add to the sysctl wrapper, as above. Bruce