Re: pipex(4): protect global lists with mutex(9)
On Sun, Jun 26, 2022 at 05:43:55PM +0300, Vitaliy Makkoveev wrote: > The updated diff below. I'll commit it today later. > - /* [N] peer's address hash chain */ > - uint16_tstate; /* [N] pipex session state */ > + /* [L] peer's address hash chain */ > + uintstate; /* [L] pipex session state */ Plaese use u_int, that is more common. I didn't even know that uint exists in our headers. OK bluhm@
Re: pipex(4): protect global lists with mutex(9)
On Sun, Jun 26, 2022 at 02:37:41PM +0200, Alexander Bluhm wrote:
> On Wed, Jun 22, 2022 at 02:46:32PM +0300, Vitaliy Makkoveev wrote:
> > @@ -547,9 +547,11 @@ ether_input(struct ifnet *ifp, struct mb
> >
> > if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
> > pipex_pppoe_input(m, session);
> > + pipex_rele_session(session);
> > KERNEL_UNLOCK();
> > return;
> > }
> > + pipex_rele_session(session);
> > }
> > #endif
> > if (etype == ETHERTYPE_PPPOEDISC)
>
> Within pipex_pppoe_lookup_session() we have this code:
>
> struct pipex_session *
> pipex_pppoe_lookup_session(struct mbuf *m0)
> {
> ...
> session = pipex_lookup_by_session_id(PIPEX_PROTO_PPPOE,
> pppoe.session_id);
> ...
> if (session && session->proto.pppoe.over_ifidx !=
> m0->m_pkthdr.ph_ifidx)
> session = NULL;
>
> return (session);
> }
>
> You have to call pipex_rele_session() before setting session to NULL.
>
Nice catch. Fixed.
> > @@ -152,24 +158,26 @@ struct cpumem;
> > /* pppac ip-extension session table */
> > struct pipex_session {
> > struct radix_node ps4_rn[2];
> > - /* [N] tree glue, and other values */
> > + /* [L] tree glue, and other values */
> > struct radix_node ps6_rn[2];
> > - /* [N] tree glue, and other values */
> > + /* [L] tree glue, and other values */
> > +
> > + struct refcnt pxs_refs;
>
> Can we call this pxs_refcnt? That is what most of the other code
> calls this field. Although it is already inconsistent.
>
Done.
> > 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 */
> > + LIST_ENTRY(pipex_session) session_list; /* [L] all session chain */
> > + LIST_ENTRY(pipex_session) state_list; /* [L] state list chain */
> > + LIST_ENTRY(pipex_session) id_chain; /* [L] id hash chain */
> > LIST_ENTRY(pipex_session) peer_addr_chain;
> > - /* [N] peer's address hash chain */
> > - uint16_tstate; /* [N] pipex session state */
> > + /* [L] peer's address hash chain */
> > + uint16_tstate; /* [L] pipex session state */
>
> Can we use a u_int value here? We want to mark it MP safe. Should
> be no difference in binary as the previous and next field are 32
> bit aligned. Or do we rely on the fact that both are also [L]
> protected?
>
The `idle_time', `state' and the global pipex(4) list are consistent and
use the `pipex_list_mtx' mutex(9) for protection. May be with the
following diffs I'll use per-session `pxs_mtx' mutex(9) for `idle_time',
but not this time.
But I have no objections to make `state' u_int or int.
> Otherwise OK bluhm@
>
The updated diff below. I'll commit it today later.
Index: sys/net/if_ethersubr.c
===
RCS file: /cvs/src/sys/net/if_ethersubr.c,v
retrieving revision 1.279
diff -u -p -r1.279 if_ethersubr.c
--- sys/net/if_ethersubr.c 22 Apr 2022 12:10:57 - 1.279
+++ sys/net/if_ethersubr.c 26 Jun 2022 14:27:20 -
@@ -547,9 +547,11 @@ ether_input(struct ifnet *ifp, struct mb
if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
pipex_pppoe_input(m, session);
+ pipex_rele_session(session);
KERNEL_UNLOCK();
return;
}
+ pipex_rele_session(session);
}
#endif
if (etype == ETHERTYPE_PPPOEDISC)
Index: sys/net/if_gre.c
===
RCS file: /cvs/src/sys/net/if_gre.c,v
retrieving revision 1.171
diff -u -p -r1.171 if_gre.c
--- sys/net/if_gre.c10 Mar 2021 10:21:47 - 1.171
+++ sys/net/if_gre.c26 Jun 2022 14:27:20 -
@@ -974,9 +974,15 @@ gre_input_1(struct gre_tunnel *key, stru
struct pipex_session *session;
session = pipex_pptp_lookup_session(m);
- if (session != NULL &&
- pipex_pptp_input(m, session) == NULL)
- return (NULL);
+ if (session != NULL) {
+ struct mbuf *m0;
+
+ m0 = pipex_pptp_input(m, session);
+ pipex_rele_session(session);
+
+
Re: pipex(4): protect global lists with mutex(9)
On Wed, Jun 22, 2022 at 02:46:32PM +0300, Vitaliy Makkoveev wrote:
> @@ -547,9 +547,11 @@ ether_input(struct ifnet *ifp, struct mb
>
> if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
> pipex_pppoe_input(m, session);
> + pipex_rele_session(session);
> KERNEL_UNLOCK();
> return;
> }
> + pipex_rele_session(session);
> }
> #endif
> if (etype == ETHERTYPE_PPPOEDISC)
Within pipex_pppoe_lookup_session() we have this code:
struct pipex_session *
pipex_pppoe_lookup_session(struct mbuf *m0)
{
...
session = pipex_lookup_by_session_id(PIPEX_PROTO_PPPOE,
pppoe.session_id);
...
if (session && session->proto.pppoe.over_ifidx != m0->m_pkthdr.ph_ifidx)
session = NULL;
return (session);
}
You have to call pipex_rele_session() before setting session to NULL.
> @@ -152,24 +158,26 @@ struct cpumem;
> /* pppac ip-extension session table */
> struct pipex_session {
> struct radix_node ps4_rn[2];
> - /* [N] tree glue, and other values */
> + /* [L] tree glue, and other values */
> struct radix_node ps6_rn[2];
> - /* [N] tree glue, and other values */
> + /* [L] tree glue, and other values */
> +
> + struct refcnt pxs_refs;
Can we call this pxs_refcnt? That is what most of the other code
calls this field. Although it is already inconsistent.
> 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 */
> + LIST_ENTRY(pipex_session) session_list; /* [L] all session chain */
> + LIST_ENTRY(pipex_session) state_list; /* [L] state list chain */
> + LIST_ENTRY(pipex_session) id_chain; /* [L] id hash chain */
> LIST_ENTRY(pipex_session) peer_addr_chain;
> - /* [N] peer's address hash chain */
> - uint16_tstate; /* [N] pipex session state */
> + /* [L] peer's address hash chain */
> + uint16_tstate; /* [L] pipex session state */
Can we use a u_int value here? We want to mark it MP safe. Should
be no difference in binary as the previous and next field are 32
bit aligned. Or do we rely on the fact that both are also [L]
protected?
> #define PIPEX_STATE_INITIAL 0x
> #define PIPEX_STATE_OPENED 0x0001
> #define PIPEX_STATE_CLOSE_WAIT 0x0002
> #define PIPEX_STATE_CLOSE_WAIT2 0x0003
> #define PIPEX_STATE_CLOSED 0x0004
>
> - uint32_tidle_time; /* [N] idle time in seconds */
> + uint32_tidle_time; /* [L] idle time in seconds */
> uint16_tip_forward:1, /* [N] {en|dis}ableIP forwarding */
> ip6_forward:1, /* [I] {en|dis}able IPv6 forwarding */
> is_multicast:1, /* [I] virtual entry for multicast */
Otherwise OK bluhm@
Re: pipex(4): protect global lists with mutex(9)
Please drop previous diff. counters_read(9) could sleep, so we can't
call it with mutex(9) held.
The diff below still uses `pipex_list_mtx' mutex(9) for pipex(4) lists
protection, but for safe `session' dereference it user reference
counters.
Index: sys/net/if_ethersubr.c
===
RCS file: /cvs/src/sys/net/if_ethersubr.c,v
retrieving revision 1.279
diff -u -p -r1.279 if_ethersubr.c
--- sys/net/if_ethersubr.c 22 Apr 2022 12:10:57 - 1.279
+++ sys/net/if_ethersubr.c 22 Jun 2022 11:41:05 -
@@ -547,9 +547,11 @@ ether_input(struct ifnet *ifp, struct mb
if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
pipex_pppoe_input(m, session);
+ pipex_rele_session(session);
KERNEL_UNLOCK();
return;
}
+ pipex_rele_session(session);
}
#endif
if (etype == ETHERTYPE_PPPOEDISC)
Index: sys/net/if_gre.c
===
RCS file: /cvs/src/sys/net/if_gre.c,v
retrieving revision 1.171
diff -u -p -r1.171 if_gre.c
--- sys/net/if_gre.c10 Mar 2021 10:21:47 - 1.171
+++ sys/net/if_gre.c22 Jun 2022 11:41:05 -
@@ -974,9 +974,15 @@ gre_input_1(struct gre_tunnel *key, stru
struct pipex_session *session;
session = pipex_pptp_lookup_session(m);
- if (session != NULL &&
- pipex_pptp_input(m, session) == NULL)
- return (NULL);
+ if (session != NULL) {
+ struct mbuf *m0;
+
+ m0 = pipex_pptp_input(m, session);
+ pipex_rele_session(session);
+
+ if (m0 == NULL)
+ return (NULL);
+ }
}
#endif
break;
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.114
diff -u -p -r1.114 if_pppx.c
--- sys/net/if_pppx.c 22 Feb 2022 01:15:02 - 1.114
+++ sys/net/if_pppx.c 22 Jun 2022 11:41:05 -
@@ -1322,9 +1322,7 @@ pppacclose(dev_t dev, int flags, int mod
splx(s);
pool_put(_session_pool, sc->sc_multicast_session);
- NET_LOCK();
pipex_destroy_all_sessions(sc);
- NET_UNLOCK();
LIST_REMOVE(sc, sc_entry);
free(sc, M_DEVBUF, sizeof(*sc));
@@ -1384,13 +1382,19 @@ pppac_del_session(struct pppac_softc *sc
{
struct pipex_session *session;
- session = pipex_lookup_by_session_id(req->pcr_protocol,
+ mtx_enter(_list_mtx);
+
+ session = pipex_lookup_by_session_id_locked(req->pcr_protocol,
req->pcr_session_id);
- if (session == NULL || session->ownersc != sc)
+ if (session == NULL || session->ownersc != sc) {
+ mtx_leave(_list_mtx);
return (EINVAL);
- pipex_unlink_session(session);
+ }
+ pipex_unlink_session_locked(session);
pipex_rele_session(session);
+ mtx_leave(_list_mtx);
+
return (0);
}
@@ -1461,6 +1465,7 @@ pppac_qstart(struct ifqueue *ifq)
session = pipex_lookup_by_ip_address(ip.ip_dst);
if (session != NULL) {
pipex_ip_output(m, session);
+ pipex_rele_session(session);
m = NULL;
}
}
Index: sys/net/pipex.c
===
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.136
diff -u -p -r1.136 pipex.c
--- sys/net/pipex.c 2 Jan 2022 22:36:04 - 1.136
+++ sys/net/pipex.c 22 Jun 2022 11:41:05 -
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include
#include
@@ -79,6 +80,8 @@
#include
#include "pipex_local.h"
+struct mutex pipex_list_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);
+
struct pool pipex_session_pool;
struct pool mppe_key_pool;
@@ -88,17 +91,18 @@ struct pool mppe_key_pool;
* A atomic operation
* I immutable after creation
* N net lock
+ * L pipex_list_mtx
*/
intpipex_enable = 0; /* [N] */
struct pipex_hash_head
-pipex_session_list,/* [N] master session
list */
-pipex_close_wait_list, /* [N] expired session list */
-pipex_peer_addr_hashtable[PIPEX_HASH_SIZE],/* [N] peer's address
hash */
-pipex_id_hashtable[PIPEX_HASH_SIZE];
Re: pipex(4): protect global lists with mutex(9)
we use to enforce pppac_qstart() and
>> pppx_if_qstart() be called with netlock held doesn't work anymore for
>> pppoe sessions:
>>
>> /* XXXSMP: be sure pppac_qstart() called with NET_LOCK held */
>> ifq_set_maxlen(>if_snd, 1);
>>
>> pptp and l2tp sessions are not affected, because the pcb layer is still
>> accessed with exclusive netlock held, but the code is common for all
>> session types. This mean we can't rely on netlock and should rework
>> pipex(4) locking.
>>
>> The diff below introduces `pipex_list_mtx' mutex(9) to protect global
>> pipex(4) lists and made `session' dereference safe. It doesn't fix the
>> assertion, but only makes us sure the pppoe session can't be killed
>> concurrently while stack processes it.
>>
>> I'll protect pipex(4) session data and rework pipex(4) output with next
>> diffs.
>>
>> ok?
>>
>> Index: sys/net/if_ethersubr.c
>> ===
>> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
>> retrieving revision 1.279
>> diff -u -p -r1.279 if_ethersubr.c
>> --- sys/net/if_ethersubr.c 22 Apr 2022 12:10:57 - 1.279
>> +++ sys/net/if_ethersubr.c 20 Jun 2022 14:44:03 -
>> @@ -545,11 +545,14 @@ ether_input(struct ifnet *ifp, struct mb
>> if (pipex_enable) {
>> struct pipex_session *session;
>>
>> +mtx_enter(_list_mtx);
>> if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
>> pipex_pppoe_input(m, session);
>> +mtx_leave(_list_mtx);
>> KERNEL_UNLOCK();
>> return;
>> }
>> +mtx_leave(_list_mtx);
>> }
>> #endif
>> if (etype == ETHERTYPE_PPPOEDISC)
>> Index: sys/net/if_gre.c
>> ===
>> RCS file: /cvs/src/sys/net/if_gre.c,v
>> retrieving revision 1.171
>> diff -u -p -r1.171 if_gre.c
>> --- sys/net/if_gre.c 10 Mar 2021 10:21:47 - 1.171
>> +++ sys/net/if_gre.c 20 Jun 2022 14:44:03 -
>> @@ -973,10 +973,18 @@ gre_input_1(struct gre_tunnel *key, stru
>> if (pipex_enable) {
>> struct pipex_session *session;
>>
>> +mtx_enter(_list_mtx);
>> session = pipex_pptp_lookup_session(m);
>> -if (session != NULL &&
>> -pipex_pptp_input(m, session) == NULL)
>> -return (NULL);
>> +if (session != NULL) {
>> +struct mbuf *m0;
>> +
>> +m0 = pipex_pptp_input(m, session);
>> +mtx_leave(_list_mtx);
>> +
>> +if (m0 == NULL)
>> +return (NULL);
>> +}
>> +mtx_leave(_list_mtx);
>> }
>> #endif
>> break;
>> Index: sys/net/if_pppx.c
>> ===
>> RCS file: /cvs/src/sys/net/if_pppx.c,v
>> retrieving revision 1.114
>> diff -u -p -r1.114 if_pppx.c
>> --- sys/net/if_pppx.c22 Feb 2022 01:15:02 - 1.114
>> +++ sys/net/if_pppx.c20 Jun 2022 14:44:03 -
>> @@ -1322,9 +1322,7 @@ pppacclose(dev_t dev, int flags, int mod
>> splx(s);
>>
>> pool_put(_session_pool, sc->sc_multicast_session);
>> -NET_LOCK();
>> pipex_destroy_all_sessions(sc);
>> -NET_UNLOCK();
>>
>> LIST_REMOVE(sc, sc_entry);
>> free(sc, M_DEVBUF, sizeof(*sc));
>> @@ -1384,11 +1382,15 @@ pppac_del_session(struct pppac_softc *sc
>> {
>> struct pipex_session *session;
>>
>> +mtx_enter(_list_mtx);
>> session = pipex_lookup_by_session_id(req->pcr_protocol,
>> req->pcr_session_id);
>> -if (session == NULL || session->ownersc != sc)
>> +if (session == NULL || session->ownersc != sc) {
>> +mtx_leave(_list_mtx);
>> return (EINVAL);
>> -pipex_unlink_session(session);
>> +}
>> +pipex_unlink_session_locked(session);
>> +mtx_leave(_list_mtx);
>> pipex_rele_session(session);
>>
>> return (0);
>> @@ -1458,11 +1460,13 @@ pppac_qsta
Re: pipex(4): protect global lists with mutex(9)
On 20.6.2022. 16:46, Vitaliy Makkoveev wrote:
> And use this mutex(9) to make the `session' dereference safe.
>
> Hrvoje Popovski pointed me, he triggered netlock assertion with pipex(4)
> pppoe sessions:
>
Hi,
I can trigger this splassert with plain snapshot and with only pppoe
clients. npppd setup is without pf.
r420-1# ifconfig ix0
ix0: flags=8843 mtu 1500
lladdr 90:e2:ba:19:29:a8
index 1 priority 0 llprio 3
media: Ethernet autoselect (10GSFP+Cu full-duplex,rxpause,txpause)
status: active
inet 192.168.100.205 netmask 0xff00 broadcast 192.168.100.255
r420-1# cat /etc/npppd/npppd.conf
authentication LOCAL type local {
users-file "/etc/npppd/npppd-users"
}
tunnel PPTP protocol pptp {
}
tunnel L2TP protocol l2tp {
authentication-method pap chap mschapv2
}
tunnel PPPOE protocol pppoe {
listen on interface ix0
authentication-method pap chap
}
ipcp IPCP-L2TP {
pool-address 10.53.251.10-10.53.251.100
dns-servers 161.53.2.69
}
ipcp IPCP-PPTP {
pool-address 10.53.252.10-10.53.252.100
dns-servers 161.53.2.69
}
ipcp IPCP-PPPOE {
pool-address 10.53.253.10-10.53.253.100
}
interface pppac0 address 10.53.251.1 ipcp IPCP-L2TP
interface pppac1 address 10.53.252.1 ipcp IPCP-PPTP
interface pppac2 address 10.53.253.1 ipcp IPCP-PPPOE
bind tunnel from L2TP authenticated by LOCAL to pppac0
bind tunnel from PPTP authenticated by LOCAL to pppac1
bind tunnel from PPPOE authenticated by LOCAL to pppac2
r420-1# npppctl ses bri
Ppp Id Assigned IPv4 Username Proto Tunnel From
-- --- -
-
0 10.53.253.11hrvojepppoe11PPPoE 90:e2:ba:33:af:ec
1 10.53.253.12hrvojepppoe12PPPoE ec:f4:bb:c8:e9:88
I'm generating traffic with iperf3 from pppoe hosts to hosts in
192.168.100.0/24 network and at same time generating traffic between
pppoe hosts.
This triggers splassert after half a hour or so ...
r420-1# splassert: pipex_ppp_output: want 2 have 0
Starting stack trace...
pipex_ppp_output(fd80a45eaa00,800022782400,21) at
pipex_ppp_output+0x5e
pppac_qstart(80ec0ab0) at pppac_qstart+0x96
ifq_serialize(80ec0ab0,80ec0b90) at ifq_serialize+0xfd
taskq_thread(80030080) at taskq_thread+0x100
end trace frame: 0x0, count: 253
End of stack trace.
>>
>> r420-1# splassert: pipex_ppp_output: want 2 have 0
>> Starting stack trace...
>> pipex_ppp_output(fd80cb7c9500,800022761200,21) at
>> pipex_ppp_output+0x5e
>> pppac_qstart(80e80ab0) at pppac_qstart+0x96
>> ifq_serialize(80e80ab0,80e80b90) at ifq_serialize+0xfd
>> taskq_thread(80030080) at taskq_thread+0x100
>> end trace frame: 0x0, count: 253
>> End of stack trace.
>> splassert: pipex_ppp_output: want 2 have 0
>> Starting stack trace...
>> pipex_ppp_output(fd80c53b2300,800022761200,21) at
>> pipex_ppp_output+0x5e
>> pppac_qstart(80e80ab0) at pppac_qstart+0x96
>> ifq_serialize(80e80ab0,80e80b90) at ifq_serialize+0xfd
>> taskq_thread(80030080) at taskq_thread+0x100
>> end trace frame: 0x0, count: 253
>> End of stack trace.
>>
>>
>> r420-1# npppctl ses bri
>> Ppp Id Assigned IPv4 Username Proto Tunnel From
>> -- --- -
>> -
>> 7 10.53.251.22r6202L2TP 192.168.100.12:1701
>> 1 10.53.253.11hrvojepppoe11PPPoE 90:e2:ba:33:af:ec
>> 0 10.53.253.12hrvojepppoe12PPPoE ec:f4:bb:da:f7:f8
>
> This means the hack we use to enforce pppac_qstart() and
> pppx_if_qstart() be called with netlock held doesn't work anymore for
> pppoe sessions:
>
> /* XXXSMP: be sure pppac_qstart() called with NET_LOCK held */
> ifq_set_maxlen(>if_snd, 1);
>
> pptp and l2tp sessions are not affected, because the pcb layer is still
> accessed with exclusive netlock held, but the code is common for all
> session types. This mean we can't rely on netlock and should rework
> pipex(4) locking.
>
> The diff below introduces `pipex_list_mtx' mutex(9) to protect global
> pipex(4) lists and made `session' dereference safe. It doesn't fix the
> assertion, but only makes us sure the pppoe session can't be killed
> concurrently while stack processes it.
>
> I'll protect pipex(4) session data and rework pipex(4) output with next
> diffs.
>
> ok?
>
> Index: sys/net/if_ethersubr.c
> ===
> RCS file: /cvs/src/sys/net/if_ethersubr.c,v
> retrieving revision 1.279
> diff -u -p -r1
pipex(4): protect global lists with mutex(9)
And use this mutex(9) to make the `session' dereference safe.
Hrvoje Popovski pointed me, he triggered netlock assertion with pipex(4)
pppoe sessions:
>
> r420-1# splassert: pipex_ppp_output: want 2 have 0
> Starting stack trace...
> pipex_ppp_output(fd80cb7c9500,800022761200,21) at
> pipex_ppp_output+0x5e
> pppac_qstart(80e80ab0) at pppac_qstart+0x96
> ifq_serialize(80e80ab0,80e80b90) at ifq_serialize+0xfd
> taskq_thread(80030080) at taskq_thread+0x100
> end trace frame: 0x0, count: 253
> End of stack trace.
> splassert: pipex_ppp_output: want 2 have 0
> Starting stack trace...
> pipex_ppp_output(fd80c53b2300,800022761200,21) at
> pipex_ppp_output+0x5e
> pppac_qstart(80e80ab0) at pppac_qstart+0x96
> ifq_serialize(80e80ab0,80e80b90) at ifq_serialize+0xfd
> taskq_thread(80030080) at taskq_thread+0x100
> end trace frame: 0x0, count: 253
> End of stack trace.
>
>
> r420-1# npppctl ses bri
> Ppp Id Assigned IPv4 Username Proto Tunnel From
> -- --- -
> -
> 7 10.53.251.22r6202L2TP 192.168.100.12:1701
> 1 10.53.253.11hrvojepppoe11PPPoE 90:e2:ba:33:af:ec
> 0 10.53.253.12hrvojepppoe12PPPoE ec:f4:bb:da:f7:f8
This means the hack we use to enforce pppac_qstart() and
pppx_if_qstart() be called with netlock held doesn't work anymore for
pppoe sessions:
/* XXXSMP: be sure pppac_qstart() called with NET_LOCK held */
ifq_set_maxlen(>if_snd, 1);
pptp and l2tp sessions are not affected, because the pcb layer is still
accessed with exclusive netlock held, but the code is common for all
session types. This mean we can't rely on netlock and should rework
pipex(4) locking.
The diff below introduces `pipex_list_mtx' mutex(9) to protect global
pipex(4) lists and made `session' dereference safe. It doesn't fix the
assertion, but only makes us sure the pppoe session can't be killed
concurrently while stack processes it.
I'll protect pipex(4) session data and rework pipex(4) output with next
diffs.
ok?
Index: sys/net/if_ethersubr.c
===
RCS file: /cvs/src/sys/net/if_ethersubr.c,v
retrieving revision 1.279
diff -u -p -r1.279 if_ethersubr.c
--- sys/net/if_ethersubr.c 22 Apr 2022 12:10:57 - 1.279
+++ sys/net/if_ethersubr.c 20 Jun 2022 14:44:03 -
@@ -545,11 +545,14 @@ ether_input(struct ifnet *ifp, struct mb
if (pipex_enable) {
struct pipex_session *session;
+ mtx_enter(_list_mtx);
if ((session = pipex_pppoe_lookup_session(m)) != NULL) {
pipex_pppoe_input(m, session);
+ mtx_leave(_list_mtx);
KERNEL_UNLOCK();
return;
}
+ mtx_leave(_list_mtx);
}
#endif
if (etype == ETHERTYPE_PPPOEDISC)
Index: sys/net/if_gre.c
===
RCS file: /cvs/src/sys/net/if_gre.c,v
retrieving revision 1.171
diff -u -p -r1.171 if_gre.c
--- sys/net/if_gre.c10 Mar 2021 10:21:47 - 1.171
+++ sys/net/if_gre.c20 Jun 2022 14:44:03 -
@@ -973,10 +973,18 @@ gre_input_1(struct gre_tunnel *key, stru
if (pipex_enable) {
struct pipex_session *session;
+ mtx_enter(_list_mtx);
session = pipex_pptp_lookup_session(m);
- if (session != NULL &&
- pipex_pptp_input(m, session) == NULL)
- return (NULL);
+ if (session != NULL) {
+ struct mbuf *m0;
+
+ m0 = pipex_pptp_input(m, session);
+ mtx_leave(_list_mtx);
+
+ if (m0 == NULL)
+ return (NULL);
+ }
+ mtx_leave(_list_mtx);
}
#endif
break;
Index: sys/net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.114
diff -u -p -r1.114 if_pppx.c
--- sys/net/if_pppx.c 22 Feb 2022 01:15:02 - 1.114
+++ sys/net/if_pppx.c 20 Jun 2022 14:44:03 -
@@ -1322,9 +1322,7 @@ pppacclose(dev_t dev, int flags, int mod
splx(s);
pool_put(_session_pool, sc->sc_multicast_session);
- NET_LOCK();
pipex_destroy_all_sessions(sc);
- NET_UNLOCK();
LIST_REMOVE(sc, sc_entry);
free(sc, M_DEVBUF
