[RFC PATCH v2 00/12] Socket type control for Landlock

[Mikhail Ivanov]

6/10/2024 11:03 AM, Günther Noack wrote:
> On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote:
>> 6/4/2024 11:22 PM, Günther Noack wrote:
>> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in
>> really strange way (see landlock/limits.h):
>>
>>    #define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
>>
>> With this definition, socket access mask overlaps the fs access
>> mask in ruleset->access_masks[layer_level]. That's why
>> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open().
>>
>> So, the macro must be defined in this way:
>>
>>    #define LANDLOCK_SHIFT_ACCESS_SOCKET	(LANDLOCK_NUM_ACCESS_NET +
>>                                           LANDLOCK_NUM_ACCESS_FS)
>>
>> With this fix, open() doesn't fail in your example.
>>
>> I'm really sorry that I somehow made such a stupid typo. I will try my
>> best to make sure this doesn't happen again.
> 
> I found that we had the exact same bug with a wrongly defined "SHIFT" value in
> [1].
> 
> Maybe we should define access_masks_t as a bit-field rather than doing the
> bit-shifts by hand.  Then the compiler would keep track of the bit-offsets
> automatically.
> 
> Bit-fields have a bad reputation, but in my understanding, this is largely
> because they make it hard to control the exact bit-by-bit layout.  In our case,
> we do not need such an exact control though, and it would be fine.
> 
> To quote Linus Torvalds on [2],
> 
>    Bitfields are fine if you don't actually care about the underlying format,
>    and want gcc to just randomly assign bits, and want things to be
>    convenient in that situation.
> 
> Let me send you a proposal patch which replaces access_masks_t with a bit-field
> and removes the need for the "SHIFT" definition, which we already got wrong in
> two patch sets now.  It has the additional benefit of making the code a bit
> shorter and also removing a few static_assert()s which are now guaranteed by the
> compiler.
> 
> —Günther
> 
> [1] https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/
> [2] https://yarchive.net/comp/linux/bitfields.html

Thank you, Günther! It really looks more clear.

This patch should be applied to Landlock separately, right?