Hola Günther :)
On 4/1/23 19:19, Günther Noack wrote:
> Hello Alejandro!
>
> On Sat, Apr 01, 2023 at 12:29:35AM +0200, Alejandro Colomar wrote:
>> Hi Günther,
>>
>> On 3/24/23 18:24, Günther Noack wrote:
>>> Signed-off-by: Günther Noack <gnoack3000@xxxxxxxxx>
>>> ---
>>> man7/landlock.7 | 65 ++++++++++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 61 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/man7/landlock.7 b/man7/landlock.7
>>> index 9c305edef..d1214ba27 100644
>>> --- a/man7/landlock.7
>>> +++ b/man7/landlock.7
>>> [...]
>>> +.EX
>>> +/* Table of available file system access rights by ABI version */
>>> +__u64 landlock_fs_access_rights[] = {
>>> + (1ULL << 13) \- 1, /* ABI v1 */
>>> + (1ULL << 14) \- 1, /* ABI v2: add "refer" */
>>> + (1ULL << 15) \- 1, /* ABI v3: add "truncate" */
>>
>> Do these magic numbers have macros? Are users expected to use
>> the magic numbers directly?
>
> You are right to point this out - it's the ugly part here :-/
>
> These are bitmasks for the different access rights which are supported
> at each ABI version, so they are a bitwise OR of a long list of
> LANDLOCK_ACCESS_FS_* constants.
>
> I am aware of three ways to write this out.
> Please let me know which of these three you prefer.
> (or maybe you have an idea for another alternative?).
>
> (It feels out of scope for this documentation patch, but do you think
> these bitmasks should be defined in the uapi/linux/landlock.h header?
> You have looked at so many man pages already -- Do you happen to know
> other places in the kernel API where such a problem has come up?)
I don't remember having seen something similar in other pages.
I think defining a macro in uapi headers could be the right thing to
do. Something like LANDLOCK_ACCESS_FS_RIGHTS_MASK_ABI_{1,2,3} or
other similar name?
>
>
> 1) Make assumptions about the numbers, for brevity
> (as done in the patch I sent).
>
> __u64 landlock_fs_access_rights[] = {
> (1ULL << 13) - 1, /* ABI v1 */
> (1ULL << 14) - 1, /* ABI v2: add "refer" */
> (1ULL << 15) - 1, /* ABI v3: add "truncate" */
> }
>
> This is the shortest variant,
> but it does not use the Landlock constants from the header.
>
>
> 2) Use the constants from the header and OR them.
>
> This is the "most correct" way, but I feel that it might be too
> verbose for a documentation example. What do you think?
>
> __u64 landlock_fs_access_rights[] = {
> /* ABI v1 */
> LANDLOCK_ACCESS_FS_EXECUTE |
> LANDLOCK_ACCESS_FS_WRITE_FILE |
> LANDLOCK_ACCESS_FS_READ_FILE |
> LANDLOCK_ACCESS_FS_READ_DIR |
> LANDLOCK_ACCESS_FS_REMOVE_DIR |
> LANDLOCK_ACCESS_FS_REMOVE_FILE |
> LANDLOCK_ACCESS_FS_MAKE_CHAR |
> LANDLOCK_ACCESS_FS_MAKE_DIR |
> LANDLOCK_ACCESS_FS_MAKE_REG |
> LANDLOCK_ACCESS_FS_MAKE_SOCK |
> LANDLOCK_ACCESS_FS_MAKE_FIFO |
> LANDLOCK_ACCESS_FS_MAKE_BLOCK |
> LANDLOCK_ACCESS_FS_MAKE_SYM,
>
> /* ABI v2: Add "refer" */
> LANDLOCK_ACCESS_FS_EXECUTE |
> LANDLOCK_ACCESS_FS_WRITE_FILE |
> LANDLOCK_ACCESS_FS_READ_FILE |
> LANDLOCK_ACCESS_FS_READ_DIR |
> LANDLOCK_ACCESS_FS_REMOVE_DIR |
> LANDLOCK_ACCESS_FS_REMOVE_FILE |
> LANDLOCK_ACCESS_FS_MAKE_CHAR |
> LANDLOCK_ACCESS_FS_MAKE_DIR |
> LANDLOCK_ACCESS_FS_MAKE_REG |
> LANDLOCK_ACCESS_FS_MAKE_SOCK |
> LANDLOCK_ACCESS_FS_MAKE_FIFO |
> LANDLOCK_ACCESS_FS_MAKE_BLOCK |
> LANDLOCK_ACCESS_FS_MAKE_SYM |
> LANDLOCK_ACCESS_FS_REFER,
>
> /* ABI v3: add "truncate" */
> LANDLOCK_ACCESS_FS_EXECUTE |
> LANDLOCK_ACCESS_FS_WRITE_FILE |
> LANDLOCK_ACCESS_FS_READ_FILE |
> LANDLOCK_ACCESS_FS_READ_DIR |
> LANDLOCK_ACCESS_FS_REMOVE_DIR |
> LANDLOCK_ACCESS_FS_REMOVE_FILE |
> LANDLOCK_ACCESS_FS_MAKE_CHAR |
> LANDLOCK_ACCESS_FS_MAKE_DIR |
> LANDLOCK_ACCESS_FS_MAKE_REG |
> LANDLOCK_ACCESS_FS_MAKE_SOCK |
> LANDLOCK_ACCESS_FS_MAKE_FIFO |
> LANDLOCK_ACCESS_FS_MAKE_BLOCK |
> LANDLOCK_ACCESS_FS_MAKE_SYM,
> LANDLOCK_ACCESS_FS_REFER |
> LANDLOCK_ACCESS_FS_TRUNCATE,
> }
>
> If I were to write production code, I would probably write it out like
> that, to be explicit -- but it feels like a long code example for a
> man page? WDYT?
Similar feelings here.
>
>
> 3) Third option is the middle way,
> naming the "highest" known access right for each ABI version:
>
> __u64 landlock_fs_access_rights[] = {
> (LANDLOCK_ACCESS_FS_MAKE_SYM << 1) - 1, /* ABI v1 */
> (LANDLOCK_ACCESS_FS_REFER << 1) - 1, /* ABI v2: add "refer" */
> (LANDLOCK_ACCESS_FS_TRUNCATE << 1) - 1, /* ABI v3: add "truncate" */
> }
I'm not sure if I like this one. I'll leave it up to you to decide
the one you like. :)
>
> In this case, we still make the assumption that the supported rights
> are the "highest" right plus all of the bits in lower order places.
>
>
>>> +/* Only use the available rights in the ruleset. */
>>> +attr.handled_access_fs &= landlock_fs_access_rights[abi \- 1];
>>> +.EE
>>> +.in
>>> +.PP
>>> +The available access rights for each ABI version are listed in the
>>> +.B VERSIONS
>>> +section.
>>> +.PP
>>> +If our program needed to create hard links or rename files between different directories
>>
>> Please keep lines below 80 columns. Break lines at phrase
>> boundaries as appropriate (e.g., in this line:)
>>
>> s/ or /\nor /
>
> Thanks, applied. Will be fixed in the next patch version.
>
>
>>> +.RB ( LANDLOCK_ACCESS_FS_REFER ),
>>> +we would require the following change to the backwards compatibility logic:
>>> +Directory reparenting is not possible in a process restricted with Landlock ABI version 1.
>
> Fixed it on this line too.
>
> Thank you for the review and for applying the two earlier patches!
:)
Cheers,
Alex
> –Günther
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
