Message-ID: <aXCeYnLuKMaNmaP0@google.com>
Date: Wed, 21 Jan 2026 10:37:38 +0100
From: Günther Noack <gnoack@...gle.com>
To: Samasth Norway Ananda <samasth.norway.ananda@...cle.com>
Cc: mic@...ikod.net, linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] landlock: Add missing ABI 7 case in documentation
 example

Thanks for the updated documentation!

The comments below are mostly about the structure in the bigger
document context.  The wording seems good.

On Fri, Jan 02, 2026 at 04:27:13PM -0800, Samasth Norway Ananda wrote:
> Add the missing case 6 and case 7 handling in the ABI version
> compatibility example to properly handle ABI < 7 kernels.
> 
> Add an optional backwards compatibility section for restrict flags
> between the case analysis and landlock_restrict_self() call. The main
> tutorial example remains unchanged with
> landlock_restrict_self(ruleset_fd, 0) to keep it simple for users who
> don't need logging flags.
> 
> Also fix misleading description of the /usr rule which incorrectly
> stated it "only allow[s] reading" when the code actually allows both
> reading and executing (LANDLOCK_ACCESS_FS_EXECUTE is included in
> allowed_access).
> 
> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@...cle.com>
> ---
>  Documentation/userspace-api/landlock.rst | 35 +++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 1d0c2c15c22e..650c7b368561 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -127,6 +127,12 @@ version, and only use the available subset of access rights:
>          /* Removes LANDLOCK_SCOPE_* for ABI < 6 */
>          ruleset_attr.scoped &= ~(LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET |
>                                   LANDLOCK_SCOPE_SIGNAL);
> +        __attribute__((fallthrough));
> +    case 6:
> +        /* Removes logging flags for ABI < 7 */
> +        __attribute__((fallthrough));
> +    case 7:
> +        break;
>      }

This code does nothing, and the comment about removing logging flags
is unclear in that context without pointing to the later section.
IMHO we can either say that logging flags are removed in the code
further below, or just leave it out.

The "case 7" case is not necessary.

>  
>  This enables the creation of an inclusive ruleset that will contain our rules.
> @@ -142,8 +148,9 @@ This enables the creation of an inclusive ruleset that will contain our rules.
>      }
>  
>  We can now add a new rule to this ruleset thanks to the returned file
> -descriptor referring to this ruleset.  The rule will only allow reading the
> -file hierarchy ``/usr``.  Without another rule, write actions would then be
> +descriptor referring to this ruleset.  The rule will allow reading and
> +executing files in the ``/usr`` hierarchy.  Without another rule, write actions
> +and other operations (make_dir, remove_file, etc.) would then be
>  denied by the ruleset.  To add ``/usr`` to the ruleset, we open it with the
>  ``O_PATH`` flag and fill the &struct landlock_path_beneath_attr with this file
>  descriptor.
> @@ -191,10 +198,30 @@ number for a specific action: HTTPS connections.
>      err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>                              &net_port, 0);
>  
> +Backwards compatibility for restrict flags
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

If you were to add this header, then the section about prctl() and
landlock_restrict_self() would also now appear under "Backwards
compatibility for restrict flags", and that would be confusing.  I
would recommend to drop the header for now and clarify in the text
what it is about.  (IMHO, it is visually reasonably easy to understand
the structure of the different sections, as they always follow the
structure "text + code snippet".)

If we want to introduce a new level of headers below "Defining and
enforcing a security policy", we would have to introduce subsections
for the other steps as well, so that it is more structured.  (If we
wanted to do that though, it would probably be better to do it in a
separate commit.)


> +When passing a non-zero ``flags`` argument to ``landlock_restrict_self()``, the
> +following backwards compatibility check needs to be taken into account:
> +
> +.. code-block:: c
> +
> +    /*
> +     * Desired restriction flags, see ABI version section above.
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^

The ABI section does not talk about the restriction flags much;
I would have expected that to link to the place where the restriction
flags are documented, which would be the system call documentation
from the header?

https://docs.kernel.org/userspace-api/landlock.html#enforcing-a-ruleset

> +     * This value is only an example and differs by use case.
> +     */
> +    int restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;
> +    if (abi < 7) {
> +        /* Clear logging flags unsupported in ABI < 7 */
> +        restrict_flags &= ~(LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF |
> +                           LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON |
> +                           LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF);
> +    }
> +
>  The next step is to restrict the current thread from gaining more privileges
>  (e.g. through a SUID binary).  We now have a ruleset with the first rule
> -allowing read access to ``/usr`` while denying all other handled accesses for
> -the filesystem, and a second rule allowing HTTPS connections.
> +allowing read and execute access to ``/usr`` while denying all other handled
> +accesses for the filesystem, and a second rule allowing HTTPS connections.
>  

If you are setting a "restrict_flags" variable here, then you would
also need to use that variable in the call to landlock_restrict_self()
in the documentation below, so that the different code sections in the
documentation work together.

—Günther

Powered by blists - more mailing lists