Message-ID: <3e21551d-24c3-459f-8cee-4d85c97c0120@oracle.com>
Date: Tue, 23 Dec 2025 14:55:21 -0800
From: samasth.norway.ananda@...cle.com
To: Günther Noack <gnoack3000@...il.com>
Cc: mic@...ikod.net, gnoack@...gle.com, linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [External] : Re: [PATCH 1/3] landlock: Add missing ABI 7 case in
 documentation example



On 12/23/25 2:22 PM, Günther Noack wrote:
> Hello!
> 
> On Tue, Dec 16, 2025 at 01:02:42PM -0800, Samasth Norway Ananda wrote:
>> Add the missing case 6 and case 7 handling in the ABI version
>> compatibility example to properly handle LANDLOCK_RESTRICT_SELF_LOG_*
>> flags for kernels with ABI < 7.
>>
>> This introduces the supported_restrict_flags variable which is
>> initialized with LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON, removed
>> in case 6 for ABI < 7, and passed to landlock_restrict_self() to
>> enable logging on supported kernels.
>>
>> Also fix misleading description of the /usr rule which incorrectly
>> stated it "only allow[s] reading" when the code actually allows both
>> reading and executing (LANDLOCK_ACCESS_FS_EXECUTE is included in
>> allowed_access).
>>
>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@...cle.com>
> 
> Thank you for sending a patch, much appreciated!
> 
> You are right to point this out - the logging aspect is a bit hard to
> spot when reading the current documentation starting from the code
> example.
> 
> 
>> ---
>>   Documentation/userspace-api/landlock.rst | 22 ++++++++++++++++++----
>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
>> index 1d0c2c15c22e..b8caac299056 100644
>> --- a/Documentation/userspace-api/landlock.rst
>> +++ b/Documentation/userspace-api/landlock.rst
>> @@ -97,6 +97,8 @@ version, and only use the available subset of access rights:
>>   .. code-block:: c
>>   
>>       int abi;
>> +    /* Tracks which landlock_restrict_self() flags are supported */
>> +    int supported_restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;
> 
> This might be confusing, as there are actually more supported flags:
> ABI v7 does not only introduce LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON,
> but also LANDLOCK_RESTRICT_SELF_LOG_SAME_EXEC_OFF and
> LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF.
> 
> I am unconvinced whether it is a good idea to set these flags in the
> first example that we have in the documentation, especially if we
> don't discuss what these flags do there.  If we suggest the wrong
> default flags in the example, people might use them in their
> production code without realizing what they do.  The way that the
> logging flags are designed, the assumption was that most users should
> be able to pass 0 as flags to landlock_restrict_self() and still get
> the relevant parts of the audit logging.
> 
> But you are right that an implementation that *does pass* logging
> flags will want check the ABI and not pass them on older kernels.
> 
> To throw in a constructive suggestion: we could also have "backwards
> compatibility for restrict flags" section between the existing case
> analysis and the landlock_restrict_self() call?  It could then say
> something like
> 
>      When passing a non-zero `flags` argument to
>      landlock_restrict_self(), the following backwards compatibility
>      check needs to be taken into account:
> 
>        /*
>         * Desired restriction flags, see section suchandsuch.
>         * This value is only an example and differs by use case.
>         */
>        int restrict_flags = LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;
>        if (abi < 7) {
>          /* clear the necessary bits */
>          restrict_flags &= ~...;
>        }
> 
> Readers who do not need to pass any flags could then skip over that
> section (I assume these are most readers).  The readers who *do* want
> to pass flags could merge that logic into the bigger case analysis
> themselves, but for the sake of explaining it we would not mix up that
> explanation with the access right discussion that much.
> 
> WDYT?

Ah got it. Thanks for pointing it out Günther.
I agree with your suggestion. I will update the documentation to follow 
that approach and send out a v2 soon

> 
>>       abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
>>       if (abi < 0) {
>> @@ -127,6 +129,17 @@ version, and only use the available subset of access rights:
>>           /* Removes LANDLOCK_SCOPE_* for ABI < 6 */
>>           ruleset_attr.scoped &= ~(LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET |
>>                                    LANDLOCK_SCOPE_SIGNAL);
>> +        __attribute__((fallthrough));
>> +    case 6:
>> +        /*
>> +         * Removes LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON for ABI < 7.
>> +         * Note: This modifies supported_restrict_flags, not ruleset_attr,
>> +         * because logging flags are passed to landlock_restrict_self().
>> +         */
>> +        supported_restrict_flags &= ~LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON;
> 
> If this should be a generic example, we would have to clear the other
> two flags here as well.

To avoid confusion. I'll follow the above approach you suggested and 
remove this part from the case section.

Thanks,
Samasth.

> 
>> +        __attribute__((fallthrough));
>> +    case 7:
>> +        break;
>>       }
> 
> Thanks,
> –Günther

Powered by blists - more mailing lists