Re: [PATCH v2 1/2] landlock: Extend documentation for kernel support

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3577 bytes --]

Hi Mickaël, Günther,

Sorry for the delay!

On Thu, Mar 07, 2024 at 11:21:57AM +0100, Mickaël Salaün wrote:
> CCing Alejandro
> 
> On Tue, Feb 27, 2024 at 05:32:20PM +0100, Günther Noack wrote:
> > On Tue, Feb 27, 2024 at 12:05:49PM +0100, Mickaël Salaün wrote:
> > > Extend the kernel support section with one subsection for build time
> > > configuration and another for boot time configuration.
> > > 
> > > Extend the boot time subsection with a concrete example.
> > > 
> > > Update the journalctl command to include the boot option.
> > > 
> > > Cc: Günther Noack <gnoack@google.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > ---
> > > 
> > > Changes since v1:
> > > * New patch, suggested by Kees Cook.
> > > ---
> > >  Documentation/userspace-api/landlock.rst | 57 +++++++++++++++++++++---
> > >  1 file changed, 51 insertions(+), 6 deletions(-)

[...]

> > > +
> > > +  lsm=landlock,lockdown,yama,integrity,apparmor
> > > +
> > > +After a reboot, we can check that Landlock is up and running by looking at
> > > +kernel logs:
> > > +
> > > +.. code-block:: console
> > > +
> > > +    # dmesg | grep landlock || journalctl -kb -g landlock
> > > +    [    0.000000] Command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > +    [    0.000000] Kernel command line: [...] lsm=landlock,lockdown,yama,integrity,apparmor
> > > +    [    0.000000] LSM: initializing lsm=lockdown,capability,landlock,yama,integrity,apparmor
> > > +    [    0.000000] landlock: Up and running.
> > > +
> > > +Note that according to the built time kernel configuration,
> > 
> > s/built time/build time/
> >                  ^
> 
> OK

Here, this should actually be "build-time" since it works as an
adjective.

> 
> > 
> > It feels like the phrase "according to" could be slightly more specific here.
> > 
> > To paraphrase Alejandro Colomar, "Note that" is usually redundant.
> > https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
> > 
> > I'd suggest:
> > 
> >   The kernel may be configured at build time to always load the ``lockdown`` and
> >   ``capability`` LSMs.  In that case, these LSMs will appear at the beginning of
> >   the ``LSM: initializing`` log line as well, even if they are not configured in
> >   the boot loader.

LGTM

> 
> OK, I integrated your suggestion.  I guess `capability` is not really
> considered an LSM but it would be too confusing and out of scope for an
> user documentation to explain that.
> 
> > 
> > > +``lockdown,capability,`` may always stay at the beginning of the ``LSM:
> > > +initializing lsm=`` list even if they are not configured with the bootloader,
> > 
> > Nit: The man pages spell this in two words as "boot loader".
> 
> OK, I'll use "boot loader" too.
> 
> > 
> > 
> > > +which is OK.
> > > +
> > > +Network support
> > > +---------------
> > > +
> > >  To be able to explicitly allow TCP operations (e.g., adding a network rule with
> > >  ``LANDLOCK_ACCESS_NET_BIND_TCP``), the kernel must support TCP
> > >  (``CONFIG_INET=y``).  Otherwise, sys_landlock_add_rule() returns an
> > > 
> > > base-commit: b4007fd27206c478a4b76e299bddf4a71787f520
> > > -- 
> > > 2.44.0
> > > 
> > 
> > Reviewed-by: Günther Noack <gnoack@google.com>
> 
> Thanks!

Reviewed-by: Alejandro Colomar <alx@kernel.org>

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]