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
