Re: [PATCH v5 3/3] landlock.7: Explain the best-effort fallback mechanism in the example

["Günther Noack" <gnoack3000@gmail.com>]

Hello Alejandro!

On Sat, Apr 01, 2023 at 12:29:35AM +0200, Alejandro Colomar wrote:
> Hi Günther,
> 
> On 3/24/23 18:24, Günther Noack wrote:
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >  man7/landlock.7 | 65 ++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 61 insertions(+), 4 deletions(-)
> > 
> > diff --git a/man7/landlock.7 b/man7/landlock.7
> > index 9c305edef..d1214ba27 100644
> > --- a/man7/landlock.7
> > +++ b/man7/landlock.7
> > [...]
> > +.EX
> > +/* Table of available file system access rights by ABI version */
> > +__u64 landlock_fs_access_rights[] = {
> > +    (1ULL << 13) \- 1,  /* ABI v1                 */
> > +    (1ULL << 14) \- 1,  /* ABI v2: add "refer"    */
> > +    (1ULL << 15) \- 1,  /* ABI v3: add "truncate" */
> 
> Do these magic numbers have macros?  Are users expected to use
> the magic numbers directly?

You are right to point this out - it's the ugly part here :-/

These are bitmasks for the different access rights which are supported
at each ABI version, so they are a bitwise OR of a long list of
LANDLOCK_ACCESS_FS_* constants.

I am aware of three ways to write this out.
Please let me know which of these three you prefer.
(or maybe you have an idea for another alternative?).

(It feels out of scope for this documentation patch, but do you think
these bitmasks should be defined in the uapi/linux/landlock.h header?
You have looked at so many man pages already -- Do you happen to know
other places in the kernel API where such a problem has come up?)


1) Make assumptions about the numbers, for brevity
   (as done in the patch I sent).

__u64 landlock_fs_access_rights[] = {
    (1ULL << 13) - 1,  /* ABI v1                 */
    (1ULL << 14) - 1,  /* ABI v2: add "refer"    */
    (1ULL << 15) - 1,  /* ABI v3: add "truncate" */
}

This is the shortest variant,
but it does not use the Landlock constants from the header.


2) Use the constants from the header and OR them.

This is the "most correct" way, but I feel that it might be too
verbose for a documentation example.  What do you think?

__u64 landlock_fs_access_rights[] = {
    /* ABI v1 */
    LANDLOCK_ACCESS_FS_EXECUTE |
    LANDLOCK_ACCESS_FS_WRITE_FILE |
    LANDLOCK_ACCESS_FS_READ_FILE |
    LANDLOCK_ACCESS_FS_READ_DIR |
    LANDLOCK_ACCESS_FS_REMOVE_DIR |
    LANDLOCK_ACCESS_FS_REMOVE_FILE |
    LANDLOCK_ACCESS_FS_MAKE_CHAR |
    LANDLOCK_ACCESS_FS_MAKE_DIR |
    LANDLOCK_ACCESS_FS_MAKE_REG |
    LANDLOCK_ACCESS_FS_MAKE_SOCK |
    LANDLOCK_ACCESS_FS_MAKE_FIFO |
    LANDLOCK_ACCESS_FS_MAKE_BLOCK |
    LANDLOCK_ACCESS_FS_MAKE_SYM,

    /* ABI v2: Add "refer" */
    LANDLOCK_ACCESS_FS_EXECUTE |
    LANDLOCK_ACCESS_FS_WRITE_FILE |
    LANDLOCK_ACCESS_FS_READ_FILE |
    LANDLOCK_ACCESS_FS_READ_DIR |
    LANDLOCK_ACCESS_FS_REMOVE_DIR |
    LANDLOCK_ACCESS_FS_REMOVE_FILE |
    LANDLOCK_ACCESS_FS_MAKE_CHAR |
    LANDLOCK_ACCESS_FS_MAKE_DIR |
    LANDLOCK_ACCESS_FS_MAKE_REG |
    LANDLOCK_ACCESS_FS_MAKE_SOCK |
    LANDLOCK_ACCESS_FS_MAKE_FIFO |
    LANDLOCK_ACCESS_FS_MAKE_BLOCK |
    LANDLOCK_ACCESS_FS_MAKE_SYM |
    LANDLOCK_ACCESS_FS_REFER,
    
    /* ABI v3: add "truncate" */
    LANDLOCK_ACCESS_FS_EXECUTE |
    LANDLOCK_ACCESS_FS_WRITE_FILE |
    LANDLOCK_ACCESS_FS_READ_FILE |
    LANDLOCK_ACCESS_FS_READ_DIR |
    LANDLOCK_ACCESS_FS_REMOVE_DIR |
    LANDLOCK_ACCESS_FS_REMOVE_FILE |
    LANDLOCK_ACCESS_FS_MAKE_CHAR |
    LANDLOCK_ACCESS_FS_MAKE_DIR |
    LANDLOCK_ACCESS_FS_MAKE_REG |
    LANDLOCK_ACCESS_FS_MAKE_SOCK |
    LANDLOCK_ACCESS_FS_MAKE_FIFO |
    LANDLOCK_ACCESS_FS_MAKE_BLOCK |
    LANDLOCK_ACCESS_FS_MAKE_SYM,
    LANDLOCK_ACCESS_FS_REFER |
    LANDLOCK_ACCESS_FS_TRUNCATE,
}

If I were to write production code, I would probably write it out like
that, to be explicit -- but it feels like a long code example for a
man page? WDYT?


3) Third option is the middle way,
   naming the "highest" known access right for each ABI version:

__u64 landlock_fs_access_rights[] = {
    (LANDLOCK_ACCESS_FS_MAKE_SYM << 1) - 1,  /* ABI v1                 */
    (LANDLOCK_ACCESS_FS_REFER << 1) - 1,     /* ABI v2: add "refer"    */
    (LANDLOCK_ACCESS_FS_TRUNCATE << 1) - 1,  /* ABI v3: add "truncate" */
}

In this case, we still make the assumption that the supported rights
are the "highest" right plus all of the bits in lower order places.


> > +/* Only use the available rights in the ruleset. */
> > +attr.handled_access_fs &= landlock_fs_access_rights[abi \- 1];
> > +.EE
> > +.in
> > +.PP
> > +The available access rights for each ABI version are listed in the
> > +.B VERSIONS
> > +section.
> > +.PP
> > +If our program needed to create hard links or rename files between different directories
> 
> Please keep lines below 80 columns.  Break lines at phrase
> boundaries as appropriate (e.g., in this line:)
> 
> s/ or /\nor /

Thanks, applied.  Will be fixed in the next patch version.


> > +.RB ( LANDLOCK_ACCESS_FS_REFER ),
> > +we would require the following change to the backwards compatibility logic:
> > +Directory reparenting is not possible in a process restricted with Landlock ABI version 1.

Fixed it on this line too.

Thank you for the review and for applying the two earlier patches!
–Günther