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

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

Hello Alejandro!

On Fri, Apr 14, 2023 at 06:35:22PM +0200, Alejandro Colomar wrote:
> On 4/14/23 17:59, Günther Noack wrote:
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >  man7/landlock.7 | 70 ++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 66 insertions(+), 4 deletions(-)
> > 
> > diff --git a/man7/landlock.7 b/man7/landlock.7
> > index 24488465e..64bfa0752 100644
> > --- a/man7/landlock.7
> > +++ b/man7/landlock.7
> > [...]
> > +/*
> > + * Table of available file system access rights by ABI version,
> > + * numbers hardcoded to keep the example short.
> > + */
> > +__u64 landlock_fs_access_rights[] = {
> > +    (1ULL << 13) \- 1,  /* ABI v1                 */
> > +    (1ULL << 14) \- 1,  /* ABI v2: add "refer"    */
> > +    (1ULL << 15) \- 1,  /* ABI v3: add "truncate" */
> > +};
> > +
> > +int abi = landlock_create_ruleset(NULL, 0,
> > +                                  LANDLOCK_CREATE_RULESET_VERSION);
> > +if (abi <= 0) {
> > +    perror("Giving up \- No Landlock support");
> 
> Using perror(3) will already print "Operation not supported", since
> errno is ENOTSUP.  Maybe this string is redundant?  How about the
> following?
> 
> 	perror("landlock_create_ruleset");  // EOPNOTSUPP

The fallback code assumes that we don't know the kernel that we run on,
so in practice we also have to handle ENOSYS.

See https://docs.kernel.org/userspace-api/landlock.html#landlock-abi-versions

I'd suggest to just make it more explicit here that it can be two
different error codes:

if (abi <= 0) {
    /* ENOTSUP or ENOSYS */
    perror("Giving up \- No Landlock support");
}

Does that sound reasonable?


> BTW, now I checked that while in Linux ENOTSUP and EOPNOTSUPP are
> equivalent, in POSIX the latter has a connotation that it's about
> sockets.  Should we document ENOTSUP in landlock_create_ruleset(2)
> instead of EOPNOTSUPP?

EOPNOTSUP is also used in Landlock's kernel documentation,
we'd maybe have to update it there as well.
I'll have a look at what is more common.


> > +    exit(EXIT_FAILURE);
> > +}
> > +if (abi > 3)
> > +    abi = 3;
> 
> This makes the example a line shorter (see MIN(3)):
> 
> abi = MIN(abi, 3);

Thanks, good point! I'll add that.

–Günther