From owner-svn-src-head@FreeBSD.ORG Thu Jan 2 11:01:45 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 077DB2BF; Thu, 2 Jan 2014 11:01:45 +0000 (UTC) Received: from elvis.mu.org (elvis.mu.org [192.203.228.196]) by mx1.freebsd.org (Postfix) with ESMTP id E13101CA7; Thu, 2 Jan 2014 11:01:44 +0000 (UTC) Received: from Alfreds-MacBook-Pro-9.local (c-76-21-10-192.hsd1.ca.comcast.net [76.21.10.192]) by elvis.mu.org (Postfix) with ESMTPSA id 4749B1A3C1A; Thu, 2 Jan 2014 03:01:44 -0800 (PST) Message-ID: <52C54717.5000608@mu.org> Date: Thu, 02 Jan 2014 03:01:43 -0800 From: Alfred Perlstein User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Pawel Jakub Dawidek Subject: Re: svn commit: r255219 - in head: contrib/tcpdump lib/libc lib/libc/capability lib/libc/include lib/libc/sys lib/libprocstat sbin/dhclient sbin/hastd sys/amd64/linux32 sys/bsm sys/cddl/compat/opensola... References: <201309050009.r8509vsE061271@svn.freebsd.org> <67DFFD7B-01DE-4862-BED3-DD42EB92A8F4@freebsd.org> <20140102093322.GA1655@garage.freebsd.pl> <52C53F69.3040507@mu.org> <20140102104904.GB1655@garage.freebsd.pl> In-Reply-To: <20140102104904.GB1655@garage.freebsd.pl> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Stanislav Sedov , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Jan 2014 11:01:45 -0000 On 1/2/14 2:49 AM, Pawel Jakub Dawidek wrote: > On Thu, Jan 02, 2014 at 02:28:57AM -0800, Alfred Perlstein wrote: >> On 1/2/14 1:33 AM, Pawel Jakub Dawidek wrote: >>> On Wed, Jan 01, 2014 at 11:16:22PM -0800, Stanislav Sedov wrote: >>>> On Sep 4, 2013, at 5:09 PM, Pawel Jakub Dawidek wrote: >>>> >>>>> This commit also breaks compatibility with some existing Capsicum system calls, >>>>> but I see no other way to do that. This should be fine as Capsicum is still >>>>> experimental and this change is not going to 9.x. >>>> Hi! >>>> >>>> This change also increases the size of kinfo_file structure, which won’t allow >>>> programs not compiled against HEAD and working with kern.info.filedesc sysctl >>>> to run properly on HEAD (e.g. 8.x, 9.x and 10.x jails won’t run properly on HEAD, >>>> and it also broke valgrind). Is there absolutely no way to avoid extending the size >>>> of this struct? >>> Well, I made this change to have space for future cap_rights_t >>> expension. I did that change for a major branch, so we don't have to do >>> it in the middle of 10.x or to not block the work until 11.0. >>> >>> Note that the structure changed size not only because of _kf_cap_spare[3] >>> field, but also because cap_rights_t is not uint64_t anymore, it is now >>> struct that contains two uint64_t (1424 - 1392 = 4 * 8). >>> >>> I'm afraid it is too late to change it for 10.0 at this point anyway. >>> Not sure if you are aware this was merged to 10, because you write about >>> 10.x jails not working properly on HEAD. 10.x jails will work properly >>> on HEAD. >>> >>> BTW. I'd love if we stop using such structures for a running kernel. >>> We should really move to using libnv to export data like that. >> Aren't there enough bits in int _kf_ispare[4]; /* Space >> for more stuff. */ >> to make this work for the time being until you can provide an alternate >> way to fetch the cap stuff from the kernel. > I don't plan to provide alternative way to fetch the cap stuff. Well, I > implemented libnv, which can be used to reimplement how we fetch all > data like kinfo_file in a ABI friendly way, but I don't plan to modify > this specific code myself. > >> Afaik you could just remove the "spare" and steal 2 or 4 entries from >> _kf_ispare until it is sorted. > Yes, this would work for current cap_rights_t structure, at least for > i386 and amd64, but would only allow to expand the structure by one > uint64_t in the future (which might or might not be enough). The > cap_rights_t structure is designed to be expanded to 5 uint64_ts without > breaking ABI. I don't want to stuck with current cap_rights_t that is > designed to expand, but cannot be, because kinfo_file wasn't modified at > the start of a major branch. > >> Can you please make use of that and discuss merge to 10 with re@? > I'm Bccing re@, but I'm pretty sure it is too late for such a change, > especially that it breaks ABI with all 10-RCs. I'm also not changing my > mind. I'd like to structure to stay as-is. > >> It really sounds like breaking top/etc under jails is something that >> should and can be avoided. > I agree. Maybe it should be done every 10 major releases (I'm still fine > with that rule), but we cannot just stuck with it forever. > > My suggestions would be: > 1. Move to libnv. > 2. Detect that the given binary was compiled against some older version > of this structure and copy old structure to userland. Not sure if we > can do that now or not, but I'd expect we can detect that. Well I agree strongly with what you are doing except the part where 9.x jails and earlier are broken because of this change. It seems like there is a way out and you agree. Perhaps since the cap fits in the spare area we can make do for the time being? The way to do a major re-org of the kinfo_file/proc/whatever is to either use libnv as you suggest, check the binary version (as you suggest) or to make an entirely new one and make the old one kinfo_file_old and make a new way to fetch the new data as we did with the various syscalls ostatfs, ostat, etc. It still seems like we have a way out that would even give capabilities another "version" (there are enough cells in _kf_ispare for the next version of the capabilities code. -Alfred >>>>> #if defined(__amd64__) || defined(__i386__) >>>>> -#define KINFO_FILE_SIZE 1392 >>>>> +#define KINFO_FILE_SIZE 1424 >>>>> #endif >>>>> >>>>> struct kinfo_file { >>>>> @@ -389,6 +390,7 @@ >>>>> uint16_t kf_pad1; /* Round to 32 bit alignment. */ >>>>> int _kf_ispare0; /* Space for more stuff. */ >>>>> cap_rights_t kf_cap_rights; /* Capability rights. */ >>>>> + uint64_t _kf_cap_spare[3]; /* Space for future cap_rights_t. */ >>>>> int _kf_ispare[4]; /* Space for more stuff. */ >>>>> /* Truncated before copyout in sysctl */ >>>>> char kf_path[PATH_MAX]; /* Path to file, if any. */ -- Alfred Perlstein