[RFC PATCH v2 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

On 10/5/2024 9:22 PM, Günther Noack wrote:
> On Sat, Oct 05, 2024 at 08:53:55PM +0300, Mikhail Ivanov wrote:
>> On 10/5/2024 7:56 PM, Günther Noack wrote:
>>> On Wed, Aug 14, 2024 at 11:01:44AM +0800, Mikhail Ivanov wrote:
>>>> +	port = htons(inet_sk(sk)->inet_num);
>>>> +	release_sock(sk);
>>>> +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
>>>
>>> Nit: The last two lines could just be
>>>
>>>     err = check_access_socket(...);
>>>
>>> and then you would only need the release_sock(sk) call in one place.
>>> (And maybe rename the goto label accordingly.)
>> This split was done in order to not hold socket lock while doing some
>> Landlock-specific logic. It might be identical in performance to
>> your suggestion, but I thought that (1) security module should have as
>> little impact on network stack as possible and (2) it is more
>> clear that locking is performed only for a few socket state checks which
>> are not related to the access control.
>>
>> I'll add this explanation with a comment if you agree that everything is
>> correct.
> 
> 
> IMHO, when you grab a lock in this function, it is clear that you'd
> unconditionally want to release it before you return from the
> function, and that in C, the normal way to guarantee unconditional
> cleanup work would be to apply the "single exit point" rule.

Yes, these 2 release_lock()s can really raise questions when reading.

> 
> That being said, the scenario is simple enough here that it's not a
> big issue in my eyes.  It was more of a minor nit about having more
> than one place where the lock has to be released.  Either way is fine
> (and also should not require excessive comments :)).

Ok

> 
>>>> +
>>>> +release_nocheck:
>>>> +	release_sock(sk);
>>>> +	return err;
>>>> +}
> 
> –Günther