On 17/07/19(Wed) 17:34, Alexander Bluhm wrote:
> On Tue, Jul 16, 2019 at 09:01:24PM -0300, Martin Pieuchot wrote:
> > On 16/07/19(Tue) 22:45, Alexander Bluhm wrote:
> > > Hi,
> > >
> > > Convert struct pkpcb malloc(9) to pool_get(9). PCB for pfkey is
> > > only used in process context, so pass PR_WAITOK to pool_init(9).
> > > The possible sleep in pool_put(9) should not hurt, as pfkeyv2_detach()
> > > is only called by soclose(9).
> >
> > In that case should we pass PR_RWLOCK as well to pool_init(9)?
>
> I ran full regress with pool_init( PR_WAITOK|PR_RWLOCK ) on i386.
> There is no difference. I think both ways work. Is there an
> advantage to use rw_lock instead of mutex? It is not in a hot path.
> If you like, I cange it to PR_RWLOCK. I have the same diff for the
> routing socket.
The ipl of the pool is IPL_NONE. This is logical because this code is
only used in process context. However as soon as pool_put/get(9) are
used with *and* without KERNEL_LOCK() a deadlock is possible due to any
interrupt grabbing the KERNEL_LOCK(). There's two way to avoid this:
raise the ipl to IPL_MPFLOOR or PR_RWLOCK.
For both routing and pfkey, the socket lock is currently the
KERNEL_LOCK(). After analyse I don't see how pr_attach() can be moved
out from the lock without pr_detach(). So it should not really matter.
I'd say put it without the flag :) ok mpi@
> > > Index: net/pfkeyv2.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfkeyv2.c,v
> > > retrieving revision 1.197
> > > diff -u -p -r1.197 pfkeyv2.c
> > > --- net/pfkeyv2.c 4 Feb 2019 21:40:52 -0000 1.197
> > > +++ net/pfkeyv2.c 13 Jul 2019 13:59:19 -0000
> > > @@ -129,6 +129,7 @@ extern struct pool ipsec_policy_pool;
> > >
> > > extern struct radix_node_head **spd_tables;
> > >
> > > +struct pool pkpcb_pool;
> > > #define PFKEY_MSG_MAXSZ 4096
> > > const struct sockaddr pfkey_addr = { 2, PF_KEY, };
> > > struct domain pfkeydomain;
> > > @@ -251,6 +252,8 @@ pfkey_init(void)
> > > srpl_rc_init(&pkptable.pkp_rc, keycb_ref, keycb_unref, NULL);
> > > rw_init(&pkptable.pkp_lk, "pfkey");
> > > SRPL_INIT(&pkptable.pkp_list);
> > > + pool_init(&pkpcb_pool, sizeof(struct pkpcb), 0,
> > > + IPL_NONE, PR_WAITOK, "pkpcb", NULL);
> > > }
> > >
> > >
> > > @@ -266,13 +269,13 @@ pfkeyv2_attach(struct socket *so, int pr
> > > if ((so->so_state & SS_PRIV) == 0)
> > > return EACCES;
> > >
> > > - kp = malloc(sizeof(struct pkpcb), M_PCB, M_WAITOK | M_ZERO);
> > > + kp = pool_get(&pkpcb_pool, PR_WAITOK|PR_ZERO);
> > > so->so_pcb = kp;
> > > refcnt_init(&kp->kcb_refcnt);
> > >
> > > error = soreserve(so, PFKEYSNDQ, PFKEYRCVQ);
> > > if (error) {
> > > - free(kp, M_PCB, sizeof(struct pkpcb));
> > > + pool_put(&pkpcb_pool, kp);
> > > return (error);
> > > }
> > >
> > > @@ -326,7 +329,7 @@ pfkeyv2_detach(struct socket *so)
> > >
> > > so->so_pcb = NULL;
> > > KASSERT((so->so_state & SS_NOFDREF) == 0);
> > > - free(kp, M_PCB, sizeof(struct pkpcb));
> > > + pool_put(&pkpcb_pool, kp);
> > >
> > > return (0);
> > > }
> > >
>
