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...

[Alfred Perlstein]

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 <pjd at FreeBSD.org> 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