ping
> On 22 Jul 2021, at 01:56, Vitaliy Makkoveev <m...@openbsd.org> wrote:
>
> With bluhm@'s diff for parallel forwarding pipex(4) could be accessed in
> parallel through (*ifp->if_input)() -> ether_input() ->
> pipex_pppoe_input(). PPPOE pipex(4) sessions are mostly immutable except
> MPPE crypt.
>
> The diff below makes pipex(4) a little bit more parallelization
> reliable.
>
> The new per-session `pxs_mtx' mutex(9) used to protect session's
> `ccp-id' which is incremented each time we send CCP reset-request.
>
> The new per-MPPE context `pxm_mtx' mutex(9) used to protect MPPE
> context. We have two of them: one for the input and one for output path.
> With bluhm@'s diff those paths are mostly serialized, except the case
> when we send CCP reset-request. I don't see the reason to other
> pipex_pppoe_input() threads spin while we perform pipex_ccp_output().
>
> Where is no lock order limitations because those new mutex(9)'es newer
> held together.
>
> I'm not PPPOE user and I'll be happy if such user tests this diff in
> real workflow.
>
> Index: sys/net/pipex.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 pipex.c
> --- sys/net/pipex.c 20 Jul 2021 16:44:55 -0000 1.134
> +++ sys/net/pipex.c 21 Jul 2021 22:27:47 -0000
> @@ -263,6 +263,7 @@ pipex_init_session(struct pipex_session
>
> /* prepare a new session */
> session = pool_get(&pipex_session_pool, PR_WAITOK | PR_ZERO);
> + mtx_init(&session->pxs_mtx, IPL_SOFTNET);
> session->state = PIPEX_STATE_INITIAL;
> session->protocol = req->pr_protocol;
> session->session_id = req->pr_session_id;
> @@ -2099,6 +2100,7 @@ pipex_mppe_init(struct pipex_mppe *mppe,
> u_char *master_key, int has_oldkey)
> {
> memset(mppe, 0, sizeof(struct pipex_mppe));
> + mtx_init(&mppe->pxm_mtx, IPL_SOFTNET);
> if (stateless)
> mppe->stateless = 1;
> if (has_oldkey)
> @@ -2238,10 +2240,6 @@ pipex_mppe_input(struct mbuf *m0, struct
> coher_cnt &= PIPEX_COHERENCY_CNT_MASK;
> pktloss = 0;
>
> - PIPEX_MPPE_DBG((session, LOG_DEBUG, "in coher_cnt=%03x %s%s",
> - mppe->coher_cnt, (flushed) ? "[flushed]" : "",
> - (encrypt) ? "[encrypt]" : ""));
> -
> if (encrypt == 0) {
> pipex_session_log(session, LOG_DEBUG,
> "Received unexpected MPPE packet.(no ecrypt)");
> @@ -2251,6 +2249,12 @@ pipex_mppe_input(struct mbuf *m0, struct
> /* adjust mbuf */
> m_adj(m0, sizeof(coher_cnt));
>
> + mtx_enter(&mppe->pxm_mtx);
> +
> + PIPEX_MPPE_DBG((session, LOG_DEBUG, "in coher_cnt=%03x %s%s",
> + mppe->coher_cnt, (flushed) ? "[flushed]" : "",
> + (encrypt) ? "[encrypt]" : ""));
> +
> /*
> * L2TP data session may be used without sequencing, PPP frames may
> * arrive in disorder. The 'coherency counter' of MPPE detects such
> @@ -2274,6 +2278,7 @@ pipex_mppe_input(struct mbuf *m0, struct
> pipex_session_log(session, LOG_DEBUG,
> "Workaround the out-of-sequence PPP framing
> problem: "
> "%d => %d", mppe->coher_cnt, coher_cnt);
> + mtx_leave(&mppe->pxm_mtx);
> goto drop;
> }
> rewind = 1;
> @@ -2305,10 +2310,19 @@ pipex_mppe_input(struct mbuf *m0, struct
> coher_cnt &= PIPEX_COHERENCY_CNT_MASK;
> mppe->coher_cnt = coher_cnt;
> } else if (mppe->coher_cnt != coher_cnt) {
> + int ccp_id;
> +
> + mtx_leave(&mppe->pxm_mtx);
> +
> /* Send CCP ResetReq */
> PIPEX_DBG((session, LOG_DEBUG, "CCP SendResetReq"));
> - pipex_ccp_output(session, CCP_RESETREQ,
> - session->ccp_id++);
> +
> + mtx_enter(&session->pxs_mtx);
> + ccp_id=session->ccp_id;
> + session->ccp_id++;
> + mtx_leave(&session->pxs_mtx);
> +
> + pipex_ccp_output(session, CCP_RESETREQ, ccp_id);
> goto drop;
> }
> if ((coher_cnt & 0xff) == 0xff) {
> @@ -2336,6 +2350,9 @@ pipex_mppe_input(struct mbuf *m0, struct
> mppe->coher_cnt++;
> mppe->coher_cnt &= PIPEX_COHERENCY_CNT_MASK;
> }
> +
> + mtx_leave(&mppe->pxm_mtx);
> +
> if (m0->m_pkthdr.len < PIPEX_PPPMINLEN)
> goto drop;
>
> @@ -2387,6 +2404,8 @@ pipex_mppe_output(struct mbuf *m0, struc
> flushed = 0;
> encrypt = 1;
>
> + mtx_enter(&mppe->pxm_mtx);
> +
> if (mppe->stateless != 0) {
> flushed = 1;
> mppe_key_change(mppe);
> @@ -2429,6 +2448,8 @@ pipex_mppe_output(struct mbuf *m0, struc
> pipex_mppe_crypt(mppe, len, cp, cp);
> }
>
> + mtx_leave(&mppe->pxm_mtx);
> +
> pipex_ppp_output(m0, session, PPP_COMP);
>
> return;
> @@ -2455,7 +2476,9 @@ pipex_ccp_input(struct mbuf *m0, struct
> switch (code) {
> case CCP_RESETREQ:
> PIPEX_DBG((session, LOG_DEBUG, "CCP RecvResetReq"));
> + mtx_enter(&session->mppe_send.pxm_mtx);
> session->mppe_send.resetreq = 1;
> + mtx_leave(&session->mppe_send.pxm_mtx);
> #ifndef PIPEX_NO_CCP_RESETACK
> PIPEX_DBG((session, LOG_DEBUG, "CCP SendResetAck"));
> pipex_ccp_output(session, CCP_RESETACK, id);
> Index: sys/net/pipex_local.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pipex_local.h,v
> retrieving revision 1.42
> diff -u -p -r1.42 pipex_local.h
> --- sys/net/pipex_local.h 20 Jul 2021 16:44:55 -0000 1.42
> +++ sys/net/pipex_local.h 21 Jul 2021 22:27:47 -0000
> @@ -57,22 +57,25 @@
> * Locks used to protect struct members:
> * I immutable after creation
> * N net lock
> + * s this pipex_session' `pxs_mtx'
> + * m this pipex_mppe' `pxm_mtx'
> */
>
> #ifdef PIPEX_MPPE
> /* mppe rc4 key */
> struct pipex_mppe {
> + struct mutex pxm_mtx;
> int16_t stateless:1, /* [I] key change mode */
> - resetreq:1, /* [N] */
> + resetreq:1, /* [m] */
> reserved:14;
> int16_t keylenbits; /* [I] key length */
> int16_t keylen; /* [I] */
> - uint16_t coher_cnt; /* [N] cohency counter */
> - struct rc4_ctx rc4ctx; /* [N] */
> - u_char master_key[PIPEX_MPPE_KEYLEN]; /* [N] master key of MPPE */
> - u_char session_key[PIPEX_MPPE_KEYLEN]; /* [N] session key of MPPE */
> + uint16_t coher_cnt; /* [m] cohency counter */
> + struct rc4_ctx rc4ctx; /* [m] */
> + u_char master_key[PIPEX_MPPE_KEYLEN]; /* [m] master key of MPPE */
> + u_char session_key[PIPEX_MPPE_KEYLEN]; /* [m] session key of MPPE */
> u_char (*old_session_keys)[PIPEX_MPPE_KEYLEN];
> - /* [N] old session keys */
> + /* [m] old session keys */
> };
> #endif /* PIPEX_MPPE */
>
> @@ -156,6 +159,8 @@ struct pipex_session {
> /* [N] tree glue, and other values */
> struct radix_node ps6_rn[2];
> /* [N] tree glue, and other values */
> + struct mutex pxs_mtx;
> +
> LIST_ENTRY(pipex_session) session_list; /* [N] all session chain */
> LIST_ENTRY(pipex_session) state_list; /* [N] state list chain */
> LIST_ENTRY(pipex_session) id_chain; /* [N] id hash chain */
> @@ -191,7 +196,7 @@ struct pipex_session {
>
> uint32_t ppp_flags; /* [I] configure flags */
> #ifdef PIPEX_MPPE
> - int ccp_id; /* [N] CCP packet id */
> + int ccp_id; /* [s] CCP packet id */
> struct pipex_mppe
> mppe_recv, /* MPPE context for incoming */
> mppe_send; /* MPPE context for outgoing */
>
