Hello,

On Mon, Jan 01, 2024 at 08:26:00PM +0100, Alexander Bluhm wrote:
> On Mon, Jan 01, 2024 at 07:10:37PM +0100, Alexandr Nedvedicky wrote:
> >     in other words: can we use inpcb::inp_mtx in your diff? this way we
> >     can avoid introduction of global mutex.
> 
> This was also my first attempt.  Below is an earlier version of the
> diff.  It has races that I could not fix.
> 
> pf_state_key_unlink_inpcb() dereferences inp = sk->sk_inp.  There
> is no guarantee that another CPU does not change sk_inp.  This code
> is not MP safe as the mutex protects the life time of inp.
> +       if (inp == NULL)
> +               return;
> +       mtx_enter(&inp->inp_mtx);
> 
> Another problem is in pf_inp_lookup().  The inp can be freed between
> READ_ONCE() and in_pcbref().
>         if (!pf_state_key_isvalid(sk))
>                 pf_mbuf_unlink_state_key(m);
>         else
> +               inp = READ_ONCE(sk->sk_inp);
> 
>         in_pcbref(inp);
> 
> A global mutex protects inp = sk->sk_inp.  So I changed it.  Note
> that pf_inp_mtx covers only very short sections of code.  I do not
> expect much lock contention.
> 

    thanks for clarification. I understand now. I'm fine with using a
    a global mutex in pf(4). it will allow you to get progress.

    I think for a long run we will need to introduce reference counting
    for inp instances. so every packet/mbuf and state key will
    get a reference. I think we will need similar mechanism to
    what we have for state keys in pf(4) now.

    feel free to go ahead with pf_inp_mtx diff. this is OK by sashan


thanks and
regards
sashan