Re: [PATCH v8 04/12] landlock: Move unmask_layers() and init_layer_masks()

["Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>]

11/28/2022 11:25 PM, Mickaël Salaün пишет:
> 
> On 28/11/2022 04:25, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 11/17/2022 9:42 PM, Mickaël Salaün пишет:
>>>
>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>> This patch moves unmask_layers() and init_layer_masks() helpers
>>>> to ruleset.c to share with landlock network implementation in
>>>
>>> …to share them with the Landlock network implementation in
>>>
>>      Got it.
>>>
>>>> following commits.
>>>>
>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>> ---
> 
> [...]
> 
>>>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>>>> index 608ab356bc3e..50baff4fcbb4 100644
>>>> --- a/security/landlock/ruleset.h
>>>> +++ b/security/landlock/ruleset.h
>>>> @@ -34,6 +34,16 @@ typedef u16 layer_mask_t;
>>>>    /* Makes sure all layers can be checked. */
>>>>    static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
>>>>
>>>> +/*
>>>> + * All access rights that are denied by default whether they are handled or not
>>>> + * by a ruleset/layer.  This must be ORed with all ruleset->fs_access_masks[]
>>>> + * entries when we need to get the absolute handled access masks.
>>>> + */
>>>> +/* clang-format off */
>>>> +#define ACCESS_INITIALLY_DENIED ( \
>>>> +	LANDLOCK_ACCESS_FS_REFER)
>>>> +/* clang-format on */
>>>
>>> This ACCESS_INITIALLY_DENIED definition must be moved, not copied. You
>>> can rename ACCESS_INITIALLY_DENIED to ACCESS_FS_INITIALLY_DENIED and
>>> move this hunk before the access_mask_t definition.
>>>
>>     Yep. Will be fixed.
>>>
>>>> +
>>>>    /**
>>>>     * struct landlock_layer - Access rights for a given layer
>>>>     */
>>>> @@ -246,4 +256,14 @@ landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
>>>>    		LANDLOCK_SHIFT_ACCESS_FS) &
>>>>    	       LANDLOCK_MASK_ACCESS_FS;
>>>>    }
>>>> +
>>>> +bool unmask_layers(const struct landlock_rule *const rule,
>>>
>>> All public Landlock helpers must be prefixed with "landlock_"
>> 
>>     Do you mean ones which are shared between fs and net parts?
> 
> All helpers that ends up in the exported ELF symbols, so all implemented
> in the .c files with their signature defined in .h files. The static
> inlined .h helpers don't need to have such prefix if there is no conflict.

   Got it. Thanks.
> 
> 
>>>
>>>> +		   const access_mask_t access_request,
>>>> +		   layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]);
>>>> +
>>>> +access_mask_t
>>>> +init_layer_masks(const struct landlock_ruleset *const domain,
>>>> +		 const access_mask_t access_request,
>>>> +		 layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]);
>>>
>>> There is a warning generated by checkpatch.pl about this line:
>>>      WARNING: function definition argument 'layer_mask_t' should also have
>>> an identifier name
>>>
>>> I think this is a bug in checkpatch.pl
>>>
>>      I got this warn, but cant get rid of it.
>>      Also think its a bug in checkpatck.pl
> 
> Please ignore it for now. It would be nice to have a checkpatch.pl fix
> though.
> 
   Ok.
>> 
>>> Any though Andy, Joe, Dwaipayan or Lukas?
> .