Andrew G. Morgan wrote:
> [I'm sorry if I'm flogging a dead horse. I'm actually really excited
> by this functionality! :-) ]
>
> Comments inline...
>
> On Tue, Jun 2, 2009 at 7:23 AM, Serge E. Hallyn <serue@xxxxxxxxxx> wrote:
>> Quoting Andrew G. Morgan (morgan@xxxxxxxxxx):
>>> On Mon, Jun 1, 2009 at 3:18 PM, Serge E. Hallyn <serue@xxxxxxxxxx> wrote:
>>>> Quoting Andrew G. Morgan (morgan@xxxxxxxxxx):
>>>>> On Mon, Jun 1, 2009 at 6:35 AM, Serge E. Hallyn <serue@xxxxxxxxxx> wrote:
>>>>>>>> I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that
>>>>>>>> suffice?
>>>>> I can't speak for other subsystems, but it seems to me as if for the
>>>>> capabilities, I'd want to create something like this in
>>>>> include/linux/capabilities.h
>>>>>
>>>>> typedef struct checkpoint_caps_s {
>>>>> /* what goes in here is the capability code's business */
>>>>> } checkpoint_caps_t;
>>>> Sigh - Did a patch this way, but the problem is userspace needs to be
>>>> able to parse the checkpoint image, so it needs to know what this struct
>>>> looks like. So if I put it the struct definition
>>>> include/linux/capability.h, I run into a whole new set of problems
>>>> trying to compile a userspace program to do a sys_restart().
>>> Does the user space app need to be able to modify the data in some
>>> way? It seems like embedding a length with the structure or something
>>> might simplify such a user space dependency.
>> Hmm, I suppose I could do something like define struct ckpt_capabilities
>> in capabilities.h, then in checkpoint_hdr.h do
>>
>> struct ckpt_capabilities;
>> struct ckpt_cap_dummy {
>> __u64 dummies[9];
>> };
>>
>> struct ckpt_hdr_cred {
>> ...
>> union {
>> struct ckpt_capabilities r;
>> struct ckpt_cap_dummy d;
>> } caps;
>> };
>
> Yes, something like this, but perhaps:
>
> struct ckpt_caps_part_s {
> int length; /* = sizeof(struct ckpt_capabilities) */
> cap_ckpt_t data;
> } caps;
>
> and then the generic checkpoint code would do:
>
> #include <linux/capabilities.h>
> caps.ckpt_capabilities_length = cap_checkpoint_save(&caps.data);
> [...]
> cap_checkpoint_restore(caps.length, &caps.caps.data);
>
> and the capability code could opaquely deal with the details.
>
> The reason I think this is more maintainable is that its clear (to the
> capability code) what is being check-pointed and, conversely, for the
> checkpoint code it is abstracted with the responsibility for detailed
> state decisions elsewhere in the kernel.
>
> I suspect I don't understand the user space code issue sufficiently.
> But if, for some reason, the user space source code is unable to
> include the definition of cap_ckpt_t, it should be clear that parsing
> this type of data structure, given that offsets are embedded in it,
> should be straightforward.
>
As I said here:
https://lists.linux-foundation.org/pipermail/containers/2009-June/018288.html
Userspace needs to understand what's in the image to be able to
provide debug/info about the image, and to be able to convert
images from older kernel version to newer (or even vice versa if
one insists).
Oren.
>> with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d). Ugly, but
>> should suit everyone?
>
> could the checkpointing code check the return value for
> cap_checkpoint_restore() and fail the restore if it returned an error?
>
> Cheers
>
> Andrew
>
>>>> So I went part-way to what you suggested in the patchset I'm about to
>>>> send out (please see patch 6/8). I think the caps code does look
>>>> nicer in this new version.
>>> Better, but I remain concerned that the code looks hard to maintain
>>> when structured this way.
>> Why exactly? Just having the struct defined in checkpoint_hdr.h? Or
>> is there something else I'm unwittingly doing?
>>
>> thanks,
>> -serge
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
