On Thu, Jun 30, 2022 at 12:34:33PM +0300, Vitaliy Makkoveev wrote:
> On Thu, Jun 30, 2022 at 11:08:48AM +0200, Claudio Jeker wrote:
> > This diff converts the SRP list to a SMR list in rtsock.c
> > SRP is a bit strange with how it works and the SMR code is a bit easier to
> > understand. Since we can sleep in the SMR_TAILQ_FOREACH() we need to grab
> > a refcount on the route pcb so that we can leave the SMR critical section
> > and then enter the SMR critical section at the end of the loop before
> > dropping the refcount again.
> >
> > The diff does not immeditaly explode but I doubt we can exploit
> > parallelism in route_input() so this may fail at some later stage if it is
> > wrong.
> >
> > Comments from the lock critics welcome
>
> We use `so_lock' rwlock(9) to protect route domain sockets. We can't
> convert this SRP list to SMR list because we call solock() within
> foreach loop.
because of the so_lock the code uses a refcnt on the route pcb to make
sure that the object is not freed while we sleep. So that is handled by
this diff.
> We can easily crash kernel by running in parallel some "route monitor"
> commands and "while true; ifconfig vether0 create ; ifconfig vether0
> destroy; done".
That does not cause problem on my system.
> > --
> > :wq Claudio
> >
> > Index: sys/net/rtsock.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/rtsock.c,v
> > retrieving revision 1.334
> > diff -u -p -r1.334 rtsock.c
> > --- sys/net/rtsock.c 28 Jun 2022 10:01:13 -0000 1.334
> > +++ sys/net/rtsock.c 30 Jun 2022 08:02:09 -0000
> > @@ -71,7 +71,7 @@
> > #include <sys/domain.h>
> > #include <sys/pool.h>
> > #include <sys/protosw.h>
> > -#include <sys/srp.h>
> > +#include <sys/smr.h>
> >
> > #include <net/if.h>
> > #include <net/if_dl.h>
> > @@ -107,8 +107,6 @@ struct walkarg {
> > };
> >
> > void route_prinit(void);
> > -void rcb_ref(void *, void *);
> > -void rcb_unref(void *, void *);
> > int route_output(struct mbuf *, struct socket *, struct sockaddr *,
> > struct mbuf *);
> > int route_ctloutput(int, struct socket *, int, int, struct mbuf *);
> > @@ -149,7 +147,7 @@ int rt_setsource(unsigned int, struct
> > struct rtpcb {
> > struct socket *rop_socket; /* [I] */
> >
> > - SRPL_ENTRY(rtpcb) rop_list;
> > + SMR_TAILQ_ENTRY(rtpcb) rop_list;
> > struct refcnt rop_refcnt;
> > struct timeout rop_timeout;
> > unsigned int rop_msgfilter; /* [s] */
> > @@ -162,8 +160,7 @@ struct rtpcb {
> > #define sotortpcb(so) ((struct rtpcb *)(so)->so_pcb)
> >
> > struct rtptable {
> > - SRPL_HEAD(, rtpcb) rtp_list;
> > - struct srpl_rc rtp_rc;
> > + SMR_TAILQ_HEAD(, rtpcb) rtp_list;
> > struct rwlock rtp_lk;
> > unsigned int rtp_count;
> > };
> > @@ -185,29 +182,12 @@ struct rtptable rtptable;
> > void
> > route_prinit(void)
> > {
> > - srpl_rc_init(&rtptable.rtp_rc, rcb_ref, rcb_unref, NULL);
> > rw_init(&rtptable.rtp_lk, "rtsock");
> > - SRPL_INIT(&rtptable.rtp_list);
> > + SMR_TAILQ_INIT(&rtptable.rtp_list);
> > pool_init(&rtpcb_pool, sizeof(struct rtpcb), 0,
> > IPL_SOFTNET, PR_WAITOK, "rtpcb", NULL);
> > }
> >
> > -void
> > -rcb_ref(void *null, void *v)
> > -{
> > - struct rtpcb *rop = v;
> > -
> > - refcnt_take(&rop->rop_refcnt);
> > -}
> > -
> > -void
> > -rcb_unref(void *null, void *v)
> > -{
> > - struct rtpcb *rop = v;
> > -
> > - refcnt_rele_wake(&rop->rop_refcnt);
> > -}
> > -
> > int
> > route_usrreq(struct socket *so, int req, struct mbuf *m, struct mbuf *nam,
> > struct mbuf *control, struct proc *p)
> > @@ -325,8 +305,7 @@ route_attach(struct socket *so, int prot
> > so->so_options |= SO_USELOOPBACK;
> >
> > rw_enter(&rtptable.rtp_lk, RW_WRITE);
> > - SRPL_INSERT_HEAD_LOCKED(&rtptable.rtp_rc, &rtptable.rtp_list, rop,
> > - rop_list);
> > + SMR_TAILQ_INSERT_HEAD_LOCKED(&rtptable.rtp_list, rop, rop_list);
> > rtptable.rtp_count++;
> > rw_exit(&rtptable.rtp_lk);
> >
> > @@ -347,8 +326,7 @@ route_detach(struct socket *so)
> > rw_enter(&rtptable.rtp_lk, RW_WRITE);
> >
> > rtptable.rtp_count--;
> > - SRPL_REMOVE_LOCKED(&rtptable.rtp_rc, &rtptable.rtp_list, rop, rtpcb,
> > - rop_list);
> > + SMR_TAILQ_REMOVE_LOCKED(&rtptable.rtp_list, rop, rop_list);
> > rw_exit(&rtptable.rtp_lk);
> >
> > sounlock(so);
> > @@ -356,6 +334,7 @@ route_detach(struct socket *so)
> > /* wait for all references to drop */
> > refcnt_finalize(&rop->rop_refcnt, "rtsockrefs");
> > timeout_del_barrier(&rop->rop_timeout);
> > + smr_barrier();
> >
> > solock(so);
> >
> > @@ -501,7 +480,6 @@ route_input(struct mbuf *m0, struct sock
> > struct rtpcb *rop;
> > struct rt_msghdr *rtm;
> > struct mbuf *m = m0;
> > - struct srp_ref sr;
> >
> > /* ensure that we can access the rtm_type via mtod() */
> > if (m->m_len < offsetof(struct rt_msghdr, rtm_type) + 1) {
> > @@ -509,7 +487,8 @@ route_input(struct mbuf *m0, struct sock
> > return;
> > }
> >
> > - SRPL_FOREACH(rop, &sr, &rtptable.rtp_list, rop_list) {
> > + smr_read_enter();
> > + SMR_TAILQ_FOREACH(rop, &rtptable.rtp_list, rop_list) {
> > /*
> > * If route socket is bound to an address family only send
> > * messages that match the address family. Address family
> > @@ -519,7 +498,8 @@ route_input(struct mbuf *m0, struct sock
> > rop->rop_proto != sa_family)
> > continue;
> >
> > -
> > + refcnt_take(&rop->rop_refcnt);
> > + smr_read_leave();
> > so = rop->rop_socket;
> > solock(so);
> >
> > @@ -579,8 +559,10 @@ route_input(struct mbuf *m0, struct sock
> > rtm_sendup(so, m);
> > next:
> > sounlock(so);
> > + smr_read_enter();
> > + refcnt_rele_wake(&rop->rop_refcnt);
> > }
> > - SRPL_LEAVE(&sr);
> > + smr_read_leave();
> >
> > m_freem(m);
> > }
> >
>
--
:wq Claudio
