On Wed, Jul 13, 2022 at 12:49:53PM +0300, Vitaliy Makkoveev wrote:
> Use per-session `pxs_mtx' mutex(9) to protect session context. Except
> MPPE encryption, PPPOE sessions are mostly immutable, so no lock
> required for that case.
>
> Global pipex(4) data is already protected by `pipex_list_mtx', so
> pipex(4) doesn't rely on netlock anymore. Netlock could be also removed
> from pppx(4)/pppac(4) layer with next diffs.
>
> This diff doesn't contain (*if_output)() magic.
>
> PPPOE/L2TP cases were additionally tested by Hrvoje Popovski.
I don't like that pipex_ppp_input() deducts from the protocoll that
the mutex is held and should be released.
Passing down the locked variable from where it is taken may be a
bit more maintainable.
But I think it would be better to restructure the code to avoid
lock releases in functions that did not take them. Could refactoring
help to make the places where we lock more obvious?
bluhm
> Index: sys/net/if_pppx.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.118
> diff -u -p -r1.118 if_pppx.c
> --- sys/net/if_pppx.c 2 Jul 2022 08:50:42 -0000 1.118
> +++ sys/net/if_pppx.c 13 Jul 2022 09:47:24 -0000
> @@ -637,8 +637,6 @@ pppx_add_session(struct pppx_dev *pxd, s
> ifp->if_softc = pxi;
> /* ifp->if_rdomain = req->pr_rdomain; */
> if_counters_alloc(ifp);
> - /* XXXSMP: be sure pppx_if_qstart() called with NET_LOCK held */
> - ifq_set_maxlen(&ifp->if_snd, 1);
>
> /* XXXSMP breaks atomicity */
> NET_UNLOCK();
> @@ -777,15 +775,27 @@ pppx_if_qstart(struct ifqueue *ifq)
> struct ifnet *ifp = ifq->ifq_if;
> struct pppx_if *pxi = (struct pppx_if *)ifp->if_softc;
> struct mbuf *m;
> - int proto;
> + int proto, lock = 0;
> +
> + switch (pxi->pxi_session->protocol) {
> + case PIPEX_PROTO_PPTP:
> + case PIPEX_PROTO_L2TP:
> + lock = 1;
> + break;
> + }
> +
> + if (lock)
> + mtx_enter(&pxi->pxi_session->pxs_mtx);
>
> - NET_ASSERT_LOCKED();
> while ((m = ifq_dequeue(ifq)) != NULL) {
> proto = *mtod(m, int *);
> m_adj(m, sizeof(proto));
>
> pipex_ppp_output(m, pxi->pxi_session, proto);
> }
> +
> + if (lock)
> + mtx_leave(&pxi->pxi_session->pxs_mtx);
> }
>
> int
> @@ -1022,8 +1032,6 @@ pppacopen(dev_t dev, int flags, int mode
> ifp->if_output = pppac_output;
> ifp->if_qstart = pppac_qstart;
> ifp->if_ioctl = pppac_ioctl;
> - /* XXXSMP: be sure pppac_qstart() called with NET_LOCK held */
> - ifq_set_maxlen(&ifp->if_snd, 1);
>
> if_counters_alloc(ifp);
> if_attach(ifp);
> @@ -1398,7 +1406,6 @@ pppac_qstart(struct ifqueue *ifq)
> struct ip ip;
> int rv;
>
> - NET_ASSERT_LOCKED();
> while ((m = ifq_dequeue(ifq)) != NULL) {
> #if NBPFILTER > 0
> if (ifp->if_bpf) {
> Index: sys/net/pipex.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.145
> diff -u -p -r1.145 pipex.c
> --- sys/net/pipex.c 12 Jul 2022 08:58:53 -0000 1.145
> +++ sys/net/pipex.c 13 Jul 2022 09:47:24 -0000
> @@ -90,7 +90,6 @@ struct pool mppe_key_pool;
> * Locks used to protect global data
> * A atomic operation
> * I immutable after creation
> - * N net lock
> * L pipex_list_mtx
> */
>
> @@ -171,7 +170,6 @@ pipex_ioctl(void *ownersc, u_long cmd, c
> {
> int ret = 0;
>
> - NET_ASSERT_LOCKED();
> switch (cmd) {
> case PIPEXGSTAT:
> ret = pipex_get_stat((struct pipex_session_stat_req *)data,
> @@ -567,8 +565,6 @@ pipex_get_stat(struct pipex_session_stat
> struct pipex_session *session;
> int error = 0;
>
> - NET_ASSERT_LOCKED();
> -
> session = pipex_lookup_by_session_id(req->psr_protocol,
> req->psr_session_id);
> if (session == NULL)
> @@ -771,7 +767,7 @@ pipex_timer(void *ignored_arg)
> Static void
> pipex_ip_output(struct mbuf *m0, struct pipex_session *session)
> {
> - int is_idle;
> + int is_idle, lock = 0;
>
> if ((session->flags & PIPEX_SFLAGS_MULTICAST) == 0) {
> /*
> @@ -800,7 +796,20 @@ pipex_ip_output(struct mbuf *m0, struct
> goto dropped;
> }
>
> + switch (session->protocol) {
> + case PIPEX_PROTO_PPTP:
> + case PIPEX_PROTO_L2TP:
> + lock = 1;
> + break;
> + }
> +
> + if (lock)
> + mtx_enter(&session->pxs_mtx);
> +
> pipex_ppp_output(m0, session, PPP_IP);
> +
> + if (lock)
> + mtx_leave(&session->pxs_mtx);
> } else {
> struct pipex_session *session_tmp;
> struct mbuf *m;
> @@ -820,9 +829,22 @@ pipex_ip_output(struct mbuf *m0, struct
> mtx_leave(&pipex_list_mtx);
>
> m = m_copym(m0, 0, M_COPYALL, M_NOWAIT);
> - if (m != NULL)
> + if (m != NULL) {
> + switch (session_tmp->protocol) {
> + case PIPEX_PROTO_PPTP:
> + case PIPEX_PROTO_L2TP:
> + lock = 1;
> + break;
> + }
> +
> + if (lock)
> + mtx_enter(&session_tmp->pxs_mtx);
> +
> pipex_ppp_output(m, session_tmp, PPP_IP);
> - else
> +
> + if (lock)
> + mtx_leave(&session_tmp->pxs_mtx);
> + } else
> counters_inc(session_tmp->stat_counters,
> pxc_oerrors);
>
> @@ -849,8 +871,6 @@ pipex_ppp_output(struct mbuf *m0, struct
> {
> u_char *cp, hdr[16];
>
> - NET_ASSERT_LOCKED();
> -
> #ifdef PIPEX_MPPE
> if (pipex_session_is_mppe_enabled(session)) {
> if (proto == PPP_IP) {
> @@ -901,15 +921,22 @@ drop:
> Static void
> pipex_ppp_input(struct mbuf *m0, struct pipex_session *session, int
> decrypted)
> {
> - int proto, hlen = 0;
> + int proto, hlen = 0, locked = 0;
> struct mbuf *n;
>
> + switch (session->protocol) {
> + case PIPEX_PROTO_PPTP:
> + case PIPEX_PROTO_L2TP:
> + locked = 1;
> + break;
> + }
> +
> KASSERT(m0->m_pkthdr.len >= PIPEX_PPPMINLEN);
> proto = pipex_ppp_proto(m0, session, 0, &hlen);
> #ifdef PIPEX_MPPE
> if (proto == PPP_COMP) {
> if (decrypted)
> - goto drop;
> + goto drop_locked;
>
> /* checked this on ppp_common_input() already. */
> KASSERT(pipex_session_is_mppe_accepted(session));
> @@ -918,6 +945,10 @@ pipex_ppp_input(struct mbuf *m0, struct
> pipex_mppe_input(m0, session);
> return;
> }
> +
> + if (locked)
> + mtx_leave(&session->pxs_mtx);
> +
> if (proto == PPP_CCP) {
> if (decrypted)
> goto drop;
> @@ -935,7 +966,7 @@ pipex_ppp_input(struct mbuf *m0, struct
> #endif
> m_adj(m0, hlen);
> pipex_ccp_input(m0, session);
> - return;
> + goto out;
> }
> #endif
> m_adj(m0, hlen);
> @@ -956,7 +987,7 @@ pipex_ppp_input(struct mbuf *m0, struct
> */
> goto drop;
> pipex_ip_input(m0, session);
> - return;
> + break;
> #ifdef INET6
> case PPP_IPV6:
> if (!decrypted && pipex_session_is_mppe_required(session))
> @@ -966,7 +997,7 @@ pipex_ppp_input(struct mbuf *m0, struct
> */
> goto drop;
> pipex_ip6_input(m0, session);
> - return;
> + break;
> #endif
> default:
> if (decrypted)
> @@ -976,8 +1007,15 @@ pipex_ppp_input(struct mbuf *m0, struct
> goto drop;
> }
>
> +out:
> + if (locked)
> + mtx_enter(&session->pxs_mtx);
> return;
> +
> drop:
> + if (locked)
> + mtx_enter(&session->pxs_mtx);
> +drop_locked:
> m_freem(m0);
> counters_inc(session->stat_counters, pxc_ierrors);
> }
> @@ -1261,6 +1299,7 @@ pipex_pppoe_input(struct mbuf *m0, struc
> sizeof(struct pipex_pppoe_header), &pppoe);
>
> hlen = sizeof(struct ether_header) + sizeof(struct pipex_pppoe_header);
> +
> if ((m0 = pipex_common_input(session, m0, hlen, ntohs(pppoe.length)))
> == NULL)
> return (NULL);
> @@ -1327,6 +1366,8 @@ pipex_pptp_output(struct mbuf *m0, struc
> struct ip *ip;
> u_char *cp;
>
> + MUTEX_ASSERT_LOCKED(&session->pxs_mtx);
> +
> reqlen = PIPEX_IPGRE_HDRLEN + (has_seq + has_ack) * 4;
>
> len = 0;
> @@ -1480,7 +1521,6 @@ pipex_pptp_input(struct mbuf *m0, struct
> struct pipex_pptp_session *pptp_session;
> int rewind = 0;
>
> - NET_ASSERT_LOCKED();
> KASSERT(m0->m_pkthdr.len >= PIPEX_IPGRE_HDRLEN);
> pptp_session = &session->proto.pptp;
>
> @@ -1510,6 +1550,9 @@ pipex_pptp_input(struct mbuf *m0, struct
> seqp = cp;
> GETLONG(seq, cp);
> }
> +
> + mtx_enter(&session->pxs_mtx);
> +
> if (has_ack) {
> ackp = cp;
> GETLONG(ack, cp);
> @@ -1553,16 +1596,28 @@ pipex_pptp_input(struct mbuf *m0, struct
> pipex_pptp_output(NULL, session, 0, 1);
> }
>
> + /*
> + * The following pipex_common_input() will release `pxs_mtx'
> + * deep within if the packet will be consumed. In the error
> + * path lock will be held all the time. So increment `rcv_gap'
> + * here, and on the error path back it out, no atomicity will
> + * be lost in all cases.
> + */
> + if (!rewind)
> + session->proto.pptp.rcv_gap += nseq;
> if ((m0 = pipex_common_input(session, m0, hlen, ntohs(gre->len)))
> == NULL) {
> /* ok, The packet is for PIPEX */
> - if (!rewind)
> - session->proto.pptp.rcv_gap += nseq;
> + mtx_leave(&session->pxs_mtx);
> return (NULL);
> }
>
> if (rewind)
> goto out_seq;
> + else {
> + /* The packet is not ours, back out `rcv_gap'. */
> + session->proto.pptp.rcv_gap -= nseq;
> + }
>
> not_ours:
> seq--; /* revert original seq value */
> @@ -1591,6 +1646,8 @@ not_ours:
> PUTLONG(ack, ackp);
> }
>
> + mtx_leave(&session->pxs_mtx);
> +
> return (m0);
> out_seq:
> pipex_session_log(session, LOG_DEBUG,
> @@ -1599,6 +1656,7 @@ out_seq:
> pptp_session->rcv_nxt + pptp_session->maxwinsz,
> ack, pptp_session->snd_una,
> pptp_session->snd_nxt);
> + mtx_leave(&session->pxs_mtx);
>
> /* FALLTHROUGH */
> drop:
> @@ -1728,6 +1786,8 @@ pipex_pptp_userland_output(struct mbuf *
> gre = mtod(m0, struct pipex_gre_header *);
> cp = PIPEX_SEEK_NEXTHDR(gre, sizeof(struct pipex_gre_header), u_char *);
>
> + mtx_enter(&session->pxs_mtx);
> +
> /*
> * overwrite sequence numbers to adjust a gap between pipex and
> * userland.
> @@ -1748,6 +1808,8 @@ pipex_pptp_userland_output(struct mbuf *
> session->proto.pptp.rcv_acked = val32;
> }
>
> + mtx_leave(&session->pxs_mtx);
> +
> return (m0);
> }
> #endif /* PIPEX_PPTP */
> @@ -1769,6 +1831,8 @@ pipex_l2tp_output(struct mbuf *m0, struc
> #endif
> struct m_tag *mtag;
>
> + MUTEX_ASSERT_LOCKED(&session->pxs_mtx);
> +
> hlen = sizeof(struct pipex_l2tp_header) +
> ((pipex_session_is_l2tp_data_sequencing_on(session))
> ? sizeof(struct pipex_l2tp_seq_header) : 0) +
> @@ -1940,17 +2004,15 @@ pipex_l2tp_input(struct mbuf *m0, int of
> uint32_t ipsecflowinfo)
> {
> struct pipex_l2tp_session *l2tp_session;
> - int length, offset, hlen, nseq;
> - u_char *cp, *nsp, *nrp;
> + int length = 0, offset = 0, hlen, nseq;
> + u_char *cp, *nsp = NULL, *nrp = NULL;
> uint16_t flags, ns = 0, nr = 0;
> int rewind = 0;
>
> - NET_ASSERT_LOCKED();
> + mtx_enter(&session->pxs_mtx);
>
> - length = offset = ns = nr = 0;
> l2tp_session = &session->proto.l2tp;
> l2tp_session->ipsecflowinfo = ipsecflowinfo;
> - nsp = nrp = NULL;
>
> m_copydata(m0, off0, sizeof(flags), &flags);
>
> @@ -2008,15 +2070,28 @@ pipex_l2tp_input(struct mbuf *m0, int of
>
> length -= hlen + offset;
> hlen += off0 + offset;
> +
> + /*
> + * The following pipex_common_input() will release `pxs_mtx'
> + * deep within if the packet will be consumed. In the error
> + * path lock will be held all the time. So increment `nr_gap'
> + * here, and on the error path back it out, no atomicity will
> + * be lost in all cases.
> + */
> + if (!rewind)
> + session->proto.l2tp.nr_gap += nseq;
> if ((m0 = pipex_common_input(session, m0, hlen, length)) == NULL) {
> /* ok, The packet is for PIPEX */
> - if (!rewind)
> - session->proto.l2tp.nr_gap += nseq;
> + mtx_leave(&session->pxs_mtx);
> return (NULL);
> }
>
> if (rewind)
> goto out_seq;
> + else {
> + /* The packet is not ours, backout `nr_gap'. */
> + session->proto.l2tp.nr_gap -= nseq;
> + }
>
> /*
> * overwrite sequence numbers to adjust a gap between pipex and
> @@ -2042,15 +2117,18 @@ pipex_l2tp_input(struct mbuf *m0, int of
> PUTSHORT(nr, nrp);
> }
>
> + mtx_leave(&session->pxs_mtx);
> +
> return (m0);
> out_seq:
> pipex_session_log(session, LOG_DEBUG,
> "Received bad data packet: out of sequence: seq=%u(%u-) "
> "ack=%u(%u-%u)", ns, l2tp_session->nr_nxt, nr, l2tp_session->ns_una,
> l2tp_session->ns_nxt);
> -
> /* FALLTHROUGH */
> drop:
> + mtx_leave(&session->pxs_mtx);
> +
> m_freem(m0);
> counters_inc(session->stat_counters, pxc_ierrors);
>
> @@ -2175,6 +2253,8 @@ pipex_l2tp_userland_output(struct mbuf *
> ns = ntohs(seq->ns);
> nr = ntohs(seq->nr);
>
> + mtx_enter(&session->pxs_mtx);
> +
> ns += session->proto.l2tp.ns_gap;
> seq->ns = htons(ns);
> session->proto.l2tp.ns_nxt++;
> @@ -2184,6 +2264,8 @@ pipex_l2tp_userland_output(struct mbuf *
> if (SEQ16_GT(nr, session->proto.l2tp.nr_acked))
> session->proto.l2tp.nr_acked = nr;
>
> + mtx_leave(&session->pxs_mtx);
> +
> return (m0);
> }
> #endif /* PIPEX_L2TP */
> @@ -2443,11 +2525,15 @@ pipex_mppe_input(struct mbuf *m0, struct
>
> /* Send CCP ResetReq */
> PIPEX_DBG((session, LOG_DEBUG, "CCP SendResetReq"));
> +
> + if (session->protocol == PIPEX_PROTO_PPPOE)
> + mtx_enter(&session->pxs_mtx);
>
> - mtx_enter(&session->pxs_mtx);
> ccp_id = session->ccp_id;
> session->ccp_id++;
> - mtx_leave(&session->pxs_mtx);
> +
> + if (session->protocol == PIPEX_PROTO_PPPOE)
> + mtx_leave(&session->pxs_mtx);
>
> pipex_ccp_output(session, CCP_RESETREQ, ccp_id);
> goto drop;
> Index: sys/net/pipex_local.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex_local.h,v
> retrieving revision 1.48
> diff -u -p -r1.48 pipex_local.h
> --- sys/net/pipex_local.h 12 Jul 2022 08:58:53 -0000 1.48
> +++ sys/net/pipex_local.h 13 Jul 2022 09:47:24 -0000
> @@ -56,8 +56,8 @@ extern struct mutex pipex_list_mtx;
>
> /*
> * Locks used to protect struct members:
> + * A atomic operation
> * I immutable after creation
> - * N net lock
> * L pipex_list_mtx
> * s this pipex_session' `pxs_mtx'
> * m this pipex_mppe' `pxm_mtx'
> @@ -91,14 +91,14 @@ struct pipex_pppoe_session {
> #ifdef PIPEX_PPTP
> struct pipex_pptp_session {
> /* sequence number gap between pipex and userland */
> - int32_t snd_gap; /* [N] gap of our sequence */
> - int32_t rcv_gap; /* [N] gap of peer's sequence */
> - int32_t ul_snd_una; /* [N] userland send acked seq */
> -
> - uint32_t snd_nxt; /* [N] send next */
> - uint32_t rcv_nxt; /* [N] receive next */
> - uint32_t snd_una; /* [N] send acked sequence */
> - uint32_t rcv_acked; /* [N] recv acked sequence */
> + int32_t snd_gap; /* [s] gap of our sequence */
> + int32_t rcv_gap; /* [s] gap of peer's sequence */
> + int32_t ul_snd_una; /* [s] userland send acked seq */
> +
> + uint32_t snd_nxt; /* [s] send next */
> + uint32_t rcv_nxt; /* [s] receive next */
> + uint32_t snd_una; /* [s] send acked sequence */
> + uint32_t rcv_acked; /* [s] recv acked sequence */
>
> int winsz; /* [I] windows size */
> int maxwinsz; /* [I] max windows size */
> @@ -141,16 +141,16 @@ struct pipex_l2tp_session {
>
> uint32_t option_flags; /* [I] protocol options */
>
> - int16_t ns_gap; /* [N] gap between userland and pipex */
> - int16_t nr_gap; /* [N] gap between userland and pipex */
> - uint16_t ul_ns_una; /* [N] unacked sequence number (userland) */
> -
> - uint16_t ns_nxt; /* [N] next sequence number to send */
> - uint16_t ns_una; /* [N] unacked sequence number to send */
> -
> - uint16_t nr_nxt; /* [N] next sequence number to recv */
> - uint16_t nr_acked; /* [N] acked sequence number to recv */
> - uint32_t ipsecflowinfo; /* [N] IPsec SA flow id for NAT-T */
> + int16_t ns_gap; /* [s] gap between userland and pipex */
> + int16_t nr_gap; /* [s] gap between userland and pipex */
> + uint16_t ul_ns_una; /* [s] unacked sequence number (userland) */
> +
> + uint16_t ns_nxt; /* [s] next sequence number to send */
> + uint16_t ns_una; /* [s] unacked sequence number to send */
> +
> + uint16_t nr_nxt; /* [s] next sequence number to recv */
> + uint16_t nr_acked; /* [s] acked sequence number to recv */
> + uint32_t ipsecflowinfo; /* [s] IPsec SA flow id for NAT-T */
> };
> #endif /* PIPEX_L2TP */
>
> @@ -197,7 +197,7 @@ struct pipex_session {
> struct sockaddr_in6 ip6_address; /* [I] remote IPv6 address */
> int ip6_prefixlen; /* [I] remote IPv6 prefixlen */
>
> - u_int ifindex; /* [N] interface index */
> + u_int ifindex; /* [A] interface index */
> void *ownersc; /* [I] owner context */
>
> uint32_t ppp_flags; /* [I] configure flags */
