[Devel] Re: [PATCH 5/9] cr: capabilities: define checkpoint and restore fns

[Andrew G. Morgan]

[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 at us.ibm.com> wrote:
> Quoting Andrew G. Morgan (morgan at kernel.org):
>> On Mon, Jun 1, 2009 at 3:18 PM, Serge E. Hallyn <serue at us.ibm.com> wrote:
>> > Quoting Andrew G. Morgan (morgan at kernel.org):
>> >> On Mon, Jun 1, 2009 at 6:35 AM, Serge E. Hallyn <serue at us.ibm.com> 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.

> 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers