From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: pf inpcb link mutex
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: "Lorenz (xha)" <me@xha.li>, tech@openbsd.org
Date: Mon, 1 Jan 2024 19:06:13 +0100

Hello,

On Mon, Jan 01, 2024 at 05:57:58PM +0100, Alexander Bluhm wrote:
> On Mon, Jan 01, 2024 at 05:45:51PM +0100, Lorenz (xha) wrote:
> > On Mon, Jan 01, 2024 at 12:20:09AM +0100, Alexander Bluhm wrote:
> > > +	mtx_leave(&pf_inp_mtx);
> > > +        if (sk == NULL)
> > > +                return;
> > 
> > whitespace before the "if"?
> 
> fixed diff

    change looks correct to me. Though I'm bit concerned about
    design choice made in this diff. see further below.
> 
> Index: net/pf.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
> diff -u -p -r1.1190 pf.c
> --- net/pf.c	28 Dec 2023 16:21:08 -0000	1.1190
> +++ net/pf.c	1 Jan 2024 16:56:02 -0000
> @@ -112,6 +112,8 @@ struct pf_queuehead	*pf_queues_inactive;
>  
>  struct pf_status	 pf_status;
>  
> +struct mutex		 pf_inp_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);

    I would prefer to move this mutex inside the struct inpcb. I know it
    makes structure larger but it looks odd to me using global mutex to
    update members which are local to structure.

    the functions in pf(4) then can manipulate the inpcb::mtx by
    dedicated functions:
	inpcb_lock(inp);
	inpcb_unlock(inp);

    just a thought. sure we can make such change as a follow up commit
    on top of your diff.

thanks and
regards
sashan