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

["Günther Noack" <gnoack3000@gmail.com>]

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.

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 :)).

> > > +
> > > +release_nocheck:
> > > +	release_sock(sk);
> > > +	return err;
> > > +}

–Günther