On Mon, Jul 06, 2020 at 10:59:17AM +0200, Martin Pieuchot wrote:
> On 01/07/20(Wed) 22:42, Vitaliy Makkoveev wrote:
> > pipex(4) has 2 mbuf queues: `pipexinq' and `pipexoutq'. When mbuf passed
> > to pipex it goes to one of these queues and pipexintr() will be
> > scheduled to process them. pipexintr() called from `netisr' context.
> >
> > It's true for pppac(4) but for pppx(4) only incoming mbufs go to
> > `pipexinq. Outgoing mbufs go directly to stack. pppx(4) enabled in
> > npppd.conf(5) by default so I guess it's the common case of pipex(4)
> > usage.
> >
> > The code looks like there is no requirements to this delayed mbufs
> > processing, we can pass it directly to stack as we do for pppx(4)
> > outgoing traffic.
> >
> > Also we have some troubles with pipexintr() as it was described in [1].
> > It's protection of `ph_cookie'. We don't this protection this time and
> > we can't because we should brake if_get(9) logic.
> >
> > Diff below removes pipexintr(). Now all mbufs passed directly without
> > enqueueing within pipex(4). We also can destroy sessions safe in all
> > cases. We also can use if_get(9) instead using unreferenced pointers to
> > `ifnet' within pipex(4). We also avoided context switch while we
> > processing mbufs within pipex(4). We decreased latency.
> >
> > I'm seeding debian torrents with this diff an all goes well.
>
> With this diff the content of pipexintr() is no longer executed with the
> KERNEL_LOCK() held. This can be seen by following the code starting in
> ether_input().
>
> Grabbing the KERNEL_LOCK() there is not a way forward. The whole idea
> of if_input_process() is to be free of KERNEL_LOCK() to not introduce
> latency delay.
>
> So this changes implies that `pipex_session_list' and possibly other
> global data structures as well as the elements linked in those are all
> protected by the NET_LOCK(). I believe this is the easiest way forward.
>
> That said I would be comfortable with this diff going in if an audit of
> the data structures accessed in the code path starting by pipex_pppoe_input()
> has been done. That implies annotating/documenting which data structures
> are now protected by the NET_LOCK() and adding the necessary
> NET_ASSERT_LOCK(). Such audit might lead to consider some ioctl code
> path changes to now serialize on the NET_LOCK() instead of the
> KERNEL_LOCK().
>
pipex(4) is simultaneously locked by NET_LOCK() and KERNEL_LOCK() but
with two exceptions:
1. As you pointed pipex_pppoe_input() called without KERNEL_LOCK() held.
2. pppac_start() called without NET_LOCK() held. Or with NET_LOCK()
held. It depends on `if_snd' usage.
Diff below enforces pppac_start() to be called with NET_LOCK() held.
Also all externally called pipex(4) input and output routines have
NET_ASSERT_LOCKED() assertion.
Now pipex(4) is fully protected by NET_LOCK() so description of struct
members chenget too.
Index: sys/net/if_pppx.c
===================================================================
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.90
diff -u -p -r1.90 if_pppx.c
--- sys/net/if_pppx.c 24 Jun 2020 08:52:53 -0000 1.90
+++ sys/net/if_pppx.c 6 Jul 2020 11:10:17 -0000
@@ -1117,6 +1117,8 @@ pppacopen(dev_t dev, int flags, int mode
ifp->if_output = pppac_output;
ifp->if_start = pppac_start;
ifp->if_ioctl = pppac_ioctl;
+ /* XXXSMP: be sure pppac_start() called under NET_LOCK() */
+ IFQ_SET_MAXLEN(&ifp->if_snd, 1);
if_counters_alloc(ifp);
if_attach(ifp);
Index: sys/net/pipex.c
===================================================================
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.117
diff -u -p -r1.117 pipex.c
--- sys/net/pipex.c 30 Jun 2020 14:05:13 -0000 1.117
+++ sys/net/pipex.c 6 Jul 2020 11:10:17 -0000
@@ -869,6 +869,7 @@ pipex_output(struct mbuf *m0, int af, in
struct ip ip;
struct mbuf *mret;
+ NET_ASSERT_LOCKED();
session = NULL;
mret = NULL;
switch (af) {
@@ -962,6 +963,8 @@ 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) {
@@ -1355,6 +1358,7 @@ pipex_pppoe_input(struct mbuf *m0, struc
int hlen;
struct pipex_pppoe_header pppoe;
+ NET_ASSERT_LOCKED();
/* already checked at pipex_pppoe_lookup_session */
KASSERT(m0->m_pkthdr.len >= (sizeof(struct ether_header) +
sizeof(pppoe)));
@@ -1586,6 +1590,7 @@ 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;
@@ -2031,6 +2036,7 @@ pipex_l2tp_input(struct mbuf *m0, int of
uint16_t flags, ns = 0, nr = 0;
int rewind = 0;
+ NET_ASSERT_LOCKED();
length = offset = ns = nr = 0;
l2tp_session = &session->proto.l2tp;
l2tp_session->ipsecflowinfo = ipsecflowinfo;
Index: sys/net/pipex.h
===================================================================
RCS file: /cvs/src/sys/net/pipex.h,v
retrieving revision 1.23
diff -u -p -r1.23 pipex.h
--- sys/net/pipex.h 5 Jun 2020 19:50:59 -0000 1.23
+++ sys/net/pipex.h 6 Jul 2020 11:10:17 -0000
@@ -187,11 +187,11 @@ struct pipex_session;
*
* Locks used to protect struct members:
* I immutable after creation
- * k kernel lock
+ * N net lock
*/
struct pipex_iface_context {
struct ifnet *ifnet_this; /* [I] outer interface */
- u_int pipexmode; /* [k] pipex mode */
+ u_int pipexmode; /* [N] pipex mode */
/* [I] virtual pipex_session entry for multicast routing */
struct pipex_session *multicast_session;
};
Index: sys/net/pipex_local.h
===================================================================
RCS file: /cvs/src/sys/net/pipex_local.h,v
retrieving revision 1.35
diff -u -p -r1.35 pipex_local.h
--- sys/net/pipex_local.h 18 Jun 2020 14:20:12 -0000 1.35
+++ sys/net/pipex_local.h 6 Jul 2020 11:10:17 -0000
@@ -56,23 +56,23 @@
/*
* Locks used to protect struct members:
* I immutable after creation
- * k kernel lock
+ * N net lock
*/
#ifdef PIPEX_MPPE
/* mppe rc4 key */
struct pipex_mppe {
int16_t stateless:1, /* [I] key change mode */
- resetreq:1, /* [k] */
+ resetreq:1, /* [N] */
reserved:14;
int16_t keylenbits; /* [I] key length */
int16_t keylen; /* [I] */
- uint16_t coher_cnt; /* [k] cohency counter */
- struct rc4_ctx rc4ctx; /* [k] */
- u_char master_key[PIPEX_MPPE_KEYLEN]; /* [k] master key of MPPE */
- u_char session_key[PIPEX_MPPE_KEYLEN]; /* [k] session key of MPPE */
+ 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 */
u_char (*old_session_keys)[PIPEX_MPPE_KEYLEN];
- /* [k] old session keys */
+ /* [N] old session keys */
};
#endif /* PIPEX_MPPE */
@@ -85,14 +85,14 @@ struct pipex_pppoe_session {
#ifdef PIPEX_PPTP
struct pipex_pptp_session {
/* sequence number gap between pipex and userland */
- int32_t snd_gap; /* [k] gap of our sequence */
- int32_t rcv_gap; /* [k] gap of peer's sequence */
- int32_t ul_snd_una; /* [k] userland send acked seq */
-
- uint32_t snd_nxt; /* [k] send next */
- uint32_t rcv_nxt; /* [k] receive next */
- uint32_t snd_una; /* [k] send acked sequence */
- uint32_t rcv_acked; /* [k] recv acked sequence */
+ 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 */
int winsz; /* [I] windows size */
int maxwinsz; /* [I] max windows size */
@@ -135,38 +135,38 @@ struct pipex_l2tp_session {
uint32_t option_flags; /* [I] protocol options */
- int16_t ns_gap; /* [k] gap between userland and pipex */
- int16_t nr_gap; /* [k] gap between userland and pipex */
- uint16_t ul_ns_una; /* [k] unacked sequence number (userland) */
-
- uint16_t ns_nxt; /* [k] next sequence number to send */
- uint16_t ns_una; /* [k] unacked sequence number to send */
-
- uint16_t nr_nxt; /* [k] next sequence number to recv */
- uint16_t nr_acked; /* [k] acked sequence number to recv */
- uint32_t ipsecflowinfo; /* [k] IPsec SA flow id for NAT-T */
+ 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 */
};
#endif /* PIPEX_L2TP */
/* pppac ip-extension sessoin table */
struct pipex_session {
struct radix_node ps4_rn[2];
- /* [k] tree glue, and other values */
+ /* [N] tree glue, and other values */
struct radix_node ps6_rn[2];
- /* [k] tree glue, and other values */
- LIST_ENTRY(pipex_session) session_list; /* [k] all session chain */
- LIST_ENTRY(pipex_session) state_list; /* [k] state list chain */
- LIST_ENTRY(pipex_session) id_chain; /* [k] id hash chain */
+ /* [N] tree glue, and other values */
+ 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 */
LIST_ENTRY(pipex_session) peer_addr_chain;
- /* [k] peer's address hash chain */
- uint16_t state; /* [k] pipex session state */
+ /* [N] peer's address hash chain */
+ uint16_t state; /* [N] pipex session state */
#define PIPEX_STATE_INITIAL 0x0000
#define PIPEX_STATE_OPENED 0x0001
#define PIPEX_STATE_CLOSE_WAIT 0x0002
#define PIPEX_STATE_CLOSE_WAIT2 0x0003
#define PIPEX_STATE_CLOSED 0x0004
- uint16_t ip_forward:1, /* [k] {en|dis}ableIP forwarding */
+ uint16_t ip_forward:1, /* [N] {en|dis}ableIP forwarding */
ip6_forward:1, /* [I] {en|dis}able IPv6 forwarding */
is_multicast:1, /* [I] virtual entry for multicast */
reserved:13;
@@ -182,16 +182,16 @@ struct pipex_session {
struct sockaddr_in6 ip6_address; /* [I] remote IPv6 address */
int ip6_prefixlen; /* [I] remote IPv6 prefixlen */
- struct pipex_iface_context* pipex_iface; /* [I] context for interface */
+ struct pipex_iface_context* pipex_iface; /* [N] context for interface */
uint32_t ppp_flags; /* [I] configure flags */
#ifdef PIPEX_MPPE
- int ccp_id; /* [k] CCP packet id */
+ int ccp_id; /* [N] CCP packet id */
struct pipex_mppe
mppe_recv, /* MPPE context for incoming */
mppe_send; /* MPPE context for outgoing */
#endif /*PIPEXMPPE */
- struct pipex_statistics stat; /* [k] statistics */
+ struct pipex_statistics stat; /* [N] statistics */
union {
#ifdef PIPEX_PPPOE
struct pipex_pppoe_session pppoe; /* context for PPPoE */
