[CRIU] [PATCH] lsm: add a --lsm-profile flag

[Pavel Emelyanov]

On 12/04/2015 06:07 PM, Tycho Andersen wrote:
> Hi Pavel,
> 
> On Fri, Dec 04, 2015 at 03:35:22PM +0300, Pavel Emelyanov wrote:
>> On 12/03/2015 08:58 PM, Tycho Andersen wrote:
>>> In LXD, we use the container name in the LSM profile. If the container name
>>> is changed on migrate (on the host side), we want to use a different LSM
>>> profile name (a. la. --cgroup-root). This flag adds that support.
>>
>> Hi, Tycho.
>>
>> Hopefully you don't plan to have it in 1.8 :) The release is in 3 days
>> 2 of which are weekends.
> 
> Nope :). I just had this patch sitting around, and Cyrill mentioned he
> was working in this area and said I should just send it and we can
> wait to apply it until after 1.8.

OK :)

>> And some more comments inline.
>>
>>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
>>> ---
>>>  cr-restore.c         | 36 +++++++++++++++++++++---------------
>>>  crtools.c            |  6 ++++++
>>>  include/cr_options.h |  5 +++++
>>>  include/lsm.h        |  3 ++-
>>>  lsm.c                | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
>>>  5 files changed, 81 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/cr-restore.c b/cr-restore.c
>>> index 1c0c641..3c636b9 100644
>>> --- a/cr-restore.c
>>> +++ b/cr-restore.c
>>> @@ -2827,29 +2827,35 @@ static int sigreturn_restore(pid_t pid, CoreEntry *core)
>>>  	if (!creds)
>>>  		goto err;
>>>  
>>> -	if (creds->lsm_profile) {
>>> -		char *rendered;
>>> +	if (creds->lsm_profile || opts.lsm_supplied) {
>>> +		char *rendered, *profile;
>>>  		int ret;
>>>  
>>> -		if (validate_lsm(creds) < 0)
>>> +		profile = creds->lsm_profile;
>>> +		if (opts.lsm_supplied)
>>> +			profile = opts.lsm_profile;
>>> +
>>> +		if (validate_lsm(profile) < 0)
>>>  			return -1;
>>>  
>>> -		ret = render_lsm_profile(creds->lsm_profile, &rendered);
>>> -		if (ret < 0) {
>>> -			goto err_nv;
>>> -		}
>>> +		if (profile) {
>>
>> Is this if required? I cannot find the combination of creds->lsm_profile and
>> opts.lsm_profile which can lead to NULL here :(
> 
> If you pass --lsm-profile none: (i.e. don't set an lsm profile), you

Ah, yes, the none results in NULL lsm_profile.

> can get NULL here. (Admittedly the syntax is a little ugly, we could
> do a special case for --lsm-profile none if you want.)

No, this way is OK. Special case would result in some if () anyway.

>>> +			ret = render_lsm_profile(profile, &rendered);
>>> +			if (ret < 0) {
>>> +				goto err_nv;
>>> +			}
>>> +
>>> +			lsm_pos = rst_mem_cpos(RM_PRIVATE);
>>> +			lsm_profile_len = strlen(rendered);
>>> +			lsm = rst_mem_alloc(lsm_profile_len + 1, RM_PRIVATE);
>>> +			if (!lsm) {
>>> +				xfree(rendered);
>>> +				goto err_nv;
>>> +			}
>>>  
>>> -		lsm_pos = rst_mem_cpos(RM_PRIVATE);
>>> -		lsm_profile_len = strlen(rendered);
>>> -		lsm = rst_mem_alloc(lsm_profile_len + 1, RM_PRIVATE);
>>> -		if (!lsm) {
>>> +			strncpy(lsm, rendered, lsm_profile_len);
>>>  			xfree(rendered);
>>> -			goto err_nv;
>>>  		}
>>>  
>>> -		strncpy(lsm, rendered, lsm_profile_len);
>>> -		xfree(rendered);
>>> -
>>>  	}
>>>  
>>>  	if (seccomp_filters_get_rst_pos(core, &n_seccomp_filters, &seccomp_filter_pos) < 0)

>>> @@ -106,6 +107,9 @@ static int selinux_get_label(pid_t pid, char **output)
>>>  
>>>  void kerndat_lsm(void)
>>>  {
>>> +	if (name)
>>> +		return;
>>
>> Please, explain why this if is required.
> 
> We can now call this function twice on restore: once if the user
> passes --lsm-profile and once in kerndat_lsm_rst(). We don't need to
> do this detection twice, though. We could delete this if you want.

No, double detection is not great :) Just add a code comment explaining this.

-- Pavel