Re: [PATCH v6 5/5] landlock: Document Landlock's file truncation support

["Mickaël Salaün" <mic@digikod.net>]

On 12/09/2022 17:46, Günther Noack wrote:
> On Fri, Sep 09, 2022 at 03:51:35PM +0200, Mickaël Salaün wrote:
>>
>> On 08/09/2022 21:58, Günther Noack wrote:
>>> Use the LANDLOCK_ACCESS_FS_TRUNCATE flag in the tutorial.
>>>
>>> Adapt the backwards compatibility example and discussion to remove the
>>> truncation flag where needed.
>>>
>>> Point out potential surprising behaviour related to truncate.
>>>
>>> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
>>> ---
>>>    Documentation/userspace-api/landlock.rst | 62 +++++++++++++++++++++---
>>>    1 file changed, 54 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
>>> index b8ea59493964..57802fd1e09b 100644
>>> --- a/Documentation/userspace-api/landlock.rst
>>> +++ b/Documentation/userspace-api/landlock.rst
>>> @@ -8,7 +8,7 @@ Landlock: unprivileged access control
>>>    =====================================
>>>    :Author: Mickaël Salaün
>>> -:Date: May 2022
>>> +:Date: September 2022
>>>    The goal of Landlock is to enable to restrict ambient rights (e.g. global
>>>    filesystem access) for a set of processes.  Because Landlock is a stackable
>>> @@ -60,7 +60,8 @@ the need to be explicit about the denied-by-default access rights.
>>>                LANDLOCK_ACCESS_FS_MAKE_FIFO |
>>>                LANDLOCK_ACCESS_FS_MAKE_BLOCK |
>>>                LANDLOCK_ACCESS_FS_MAKE_SYM |
>>> -            LANDLOCK_ACCESS_FS_REFER,
>>> +            LANDLOCK_ACCESS_FS_REFER |
>>> +            LANDLOCK_ACCESS_FS_TRUNCATE,
>>>        };
>>>    Because we may not know on which kernel version an application will be
>>> @@ -69,16 +70,26 @@ should try to protect users as much as possible whatever the kernel they are
>>>    using.  To avoid binary enforcement (i.e. either all security features or
>>>    none), we can leverage a dedicated Landlock command to get the current version
>>>    of the Landlock ABI and adapt the handled accesses.  Let's check if we should
>>> -remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported
>>> -starting with the second version of the ABI.
>>> +remove the `LANDLOCK_ACCESS_FS_REFER` or `LANDLOCK_ACCESS_FS_TRUNCATE` access
>>> +rights, which are only supported starting with the second and third version of
>>> +the ABI.
>>>    .. code-block:: c
>>>        int abi;
>>>        abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
>>> -    if (abi < 2) {
>>> -        ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
>>> +    switch (abi) {
>>> +    case -1:
>>> +            perror("The running kernel does not enable to use Landlock");
>>> +            return 1;
>>> +    case 1:
>>> +            /* Removes LANDLOCK_ACCESS_FS_REFER for ABI < 2 */
>>> +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER;
>>> +            __attribute__((fallthrough));
>>> +    case 2:
>>> +            /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
>>> +            ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
>>>        }
>>>    This enables to create an inclusive ruleset that will contain our rules.
>>> @@ -127,8 +138,8 @@ descriptor.
>>>    It may also be required to create rules following the same logic as explained
>>>    for the ruleset creation, by filtering access rights according to the Landlock
>>> -ABI version.  In this example, this is not required because
>>> -`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule.
>>> +ABI version.  In this example, this is not required because all of the requested
>>> +``allowed_access`` rights are already available in ABI 1.
>>
>> This fix is correct, but it should not be part of this series. FYI, I have a
>> patch almost ready to fix some documentation style issues. Please remove
>> this hunk for the next series. I'll deal with the merge conflicts if any.
> 
> Can you please clarify what part of it should not be part of this
> series?

My mistake, I guess I was reviewing something else… I was thinking about 
style changes, but it is not the case here. Using "``" is correct.


> 
> In this hunk, I've started using double backquote, but I've also
> changed the meaning of the sentence slightly so that it is still
> correct when the truncate right is introduced.
> 
> It is still correct that the backwards compatibility check is not
> required because LANDLOCK_ACCESS_FS_REFER is not allowed by any rule.
> But with the new truncate flag, LANDLOCK_ACCESS_FS_TRUNCATE may also
> not be allowed by any rule so that we can skip this check.
> 
> Should I remove this hunk entirely?

Keep your changes, it's better like this.


> 
> Or maybe rather phrase it like
> 
>    It may also be required to create rules following the same logic as
>    explained for the ruleset creation, by filtering access rights
>    according to the Landlock ABI version. In this example, this is not
>    required because `LANDLOCK_ACCESS_FS_REFER` and
>    `LANDLOCK_ACCESS_FS_TRUNCATE` are not allowed by any rule.
>   
> ?