Re: [PATCH] landlock: Clarify documentation for struct landlock_ruleset_attr

["Günther Noack" <gnoack@google.com>]

On Wed, Jul 10, 2024 at 04:15:47PM +0200, Mickaël Salaün wrote:
> On Wed, Jul 10, 2024 at 12:01:34PM +0000, Günther Noack wrote:
> > Signed-off-by: Günther Noack <gnoack@google.com>
> > Cc: Alejandro Colomar <alx@kernel.org>
> > Cc: Mickaël Salaün <mic@digikod.net>
> > Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> > Cc: linux-security-module@vger.kernel.org
> > ---
> >  include/uapi/linux/landlock.h | 42 ++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index 68625e728f43..6f1b05c6995b 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -12,30 +12,32 @@
> >  #include <linux/types.h>
> >  
> >  /**
> > - * struct landlock_ruleset_attr - Ruleset definition
> > + * struct landlock_ruleset_attr - Ruleset definition.
> >   *
> > - * Argument of sys_landlock_create_ruleset().  This structure can grow in
> > - * future versions.
> > + * @handled_access_fs: Bitmask of handled filesystem actions (cf. `Filesystem flags`_)
> > + * @handled_access_net: Bitmask of handled network actions (cf. `Network flags`_)
> 
> These @handled_* lines should be kept close the the related fields to
> ease maintenance and consistency.  It looks like the Sphinx rendering
> would be the same.

Done.


> > + * Argument of sys_landlock_create_ruleset().
> > + *
> > + * This struct defines a set of *handled access rights*, a set of actions on
> > + * different object types, which should be denied by default when the ruleset is
> 
> > + * enacted.  Vice versa, access rights that are not specifically listed here are
> > + * going to be allowed when the ruleset is enacted.
> 
> They could still be denied because of other access controls or parent
> Landlock domains.

Done, reworded as

    Vice versa, access rights that are not specifically listed here are not
    going to be denied by this ruleset when it is enacted.

Now it's more technically correct, without having to add a lot more text.


> > + * One exception is the %LANDLOCK_ACCESS_FS_REFER access right, which is always
> > + * implicitly *handled*, even when its bit is not set in @handled_access_fs.
> 
> I wrote this sentence but I now think it might be confusing.
> LANDLOCK_ACCESS_FS_REFER couldn't be handled before it was introduced
> (with Linux 5.19).  I couldn't find a better way to explain it though.

I think the best description we have is in the "Filesystem flags" section
further down in this header file, where all the individual flags are explained.
I reworded it slightly and added a cross-reference to that section.

> 
> > + * However, in order to add new rules with this access right, the bit must still
> > + * be set explicitly.
> > + *
> > + * The explicit listing of *handled access rights* is required for backwards
> > + * compatibility reasons.  In most use cases, processes that use Landlock will
> > + * *handle* a wide range or all access rights that they know about at build
> > + * time.
> 
> ...and that they tested with a kernel supporting all of them.

Added.

Thank you for the review!
—Günther