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

[Mickaël Salaün]

On Mon, Mar 18, 2024 at 10:50:42AM +0100, Alejandro Colomar wrote:
> 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@xxxxxxxxxx>
> > > > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > > > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx>
> > > > ---
> > > > 
> > > > 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.

Thanks Alex but this was already merged:
https://git.kernel.org/torvalds/c/35e886e88c803920644c9d3abb45a9ecb7f1e761

Because I picked Günther's below suggestion, it should be good right?

> 
> > 
> > > 
> > > 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@xxxxxxxxx/
> > > 
> > > 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@xxxxxxxxxx>
> > 
> > Thanks!
> 
> Reviewed-by: Alejandro Colomar <alx@xxxxxxxxxx>
> 
> Have a lovely day!
> Alex
> 
> -- 
> <https://www.alejandro-colomar.es/>
> Looking for a remote C programming job at the moment.