From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oren Laadan Subject: Re: [PATCH 5/9] cr: capabilities: define checkpoint and restore fns Date: Tue, 02 Jun 2009 20:05:35 -0400 Message-ID: <4A25BE4F.6000603@cs.columbia.edu> References: <20090529223229.GA14536@us.ibm.com> <20090529223319.GE14602@us.ibm.com> <20090601013837.GA15897@hallyn.com> <551280e50905311918j28cd2482g5918bf9b0bcb297a@mail.gmail.com> <20090601133508.GA18889@us.ibm.com> <551280e50906010846i2b46c603x55eea7967233b2e0@mail.gmail.com> <20090601221857.GA29164@us.ibm.com> <551280e50906020649n4ea15ca9y3c0a22b0114b807c@mail.gmail.com> <20090602142353.GA11135@us.ibm.com> <551280e50906020849o12f777dma4fd66d0dd887e38@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <551280e50906020849o12f777dma4fd66d0dd887e38@mail.gmail.com> Sender: linux-security-module-owner@vger.kernel.org To: "Andrew G. Morgan" Cc: "Serge E. Hallyn" , Linux Containers , Alexey Dobriyan , David Howells , linux-security-module@vger.kernel.org List-Id: containers.vger.kernel.org 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 wrote: >> Quoting Andrew G. Morgan (morgan@kernel.org): >>> On Mon, Jun 1, 2009 at 3:18 PM, Serge E. Hallyn wrote: >>>> Quoting Andrew G. Morgan (morgan@kernel.org): >>>>> On Mon, Jun 1, 2009 at 6:35 AM, Serge E. Hallyn 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 > 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >