On Tue, Jul 28, 2020 at 10:26:53AM +0200, Martin Pieuchot wrote:
> On 17/07/20(Fri) 17:04, Vitaliy Makkoveev wrote:
> > Subj. Also add NET_ASSERT_LOCKED() to pipex_{link,unlink,rele}_session()
> > to be sure they called under NET_LOCK().
>
> pipex_rele_session() is freeing memory. When this function is called
> those chunks of memory shouldn't be referenced by any other CPU or piece
> of descriptor in the network stack. So the NET_LOCK() shouldn't be
> required.
Fixed.
>
> Rest of the diff is fine. I'd suggest you put the assertions just above
> the LIST_INSERT or LIST_REMOVE like it is done in other parts of the stack.
>
We should not add session to lists if `iface->pipexmode' is null. So
check and modification should be done while the same lock is held.
That's why assertion in pipex_link_session() is in the right place, just
before `iface->pipexmode' check.
I moved assertion just before LIST_REMOVE() within
pipex_unlink_session().
Updated diff below.
Index: sys/net/pipex.c
===================================================================
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.120
diff -u -p -r1.120 pipex.c
--- sys/net/pipex.c 17 Jul 2020 08:57:27 -0000 1.120
+++ sys/net/pipex.c 28 Jul 2020 09:47:51 -0000
@@ -83,19 +83,24 @@ struct pool pipex_session_pool;
struct pool mppe_key_pool;
/*
- * static/global variables
+ * Global data
+ * Locks used to protect global data
+ * A atomic operation
+ * I immutable after creation
+ * N net lock
*/
-int pipex_enable = 0;
+
+int pipex_enable = 0; /* [N] */
struct pipex_hash_head
- pipex_session_list, /* master session list
*/
- pipex_close_wait_list, /* expired session list */
- pipex_peer_addr_hashtable[PIPEX_HASH_SIZE], /* peer's address hash
*/
- pipex_id_hashtable[PIPEX_HASH_SIZE]; /* peer id hash */
+ 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]; /* [N] peer id hash */
-struct radix_node_head *pipex_rd_head4 = NULL;
-struct radix_node_head *pipex_rd_head6 = NULL;
+struct radix_node_head *pipex_rd_head4 = NULL; /* [N] */
+struct radix_node_head *pipex_rd_head6 = NULL; /* [N] */
struct timeout pipex_timer_ch; /* callout timer context */
-int pipex_prune = 1; /* walk list every seconds */
+int pipex_prune = 1; /* [I] walk list every seconds */
/* pipex traffic queue */
struct mbuf_queue pipexinq = MBUF_QUEUE_INITIALIZER(IFQ_MAXLEN, IPL_NET);
@@ -105,7 +110,7 @@ struct mbuf_queue pipexoutq = MBUF_QUEUE
#define ph_ppp_proto ether_vtag
#ifdef PIPEX_DEBUG
-int pipex_debug = 0; /* systcl net.inet.ip.pipex_debug */
+int pipex_debug = 0; /* [A] systcl net.inet.ip.pipex_debug */
#endif
/* PPP compression == MPPE is assumed, so don't answer CCP Reset-Request. */
@@ -430,6 +435,8 @@ pipex_link_session(struct pipex_session
{
struct pipex_hash_head *chain;
+ NET_ASSERT_LOCKED();
+
if (!iface->pipexmode)
return (ENXIO);
if (pipex_lookup_by_session_id(session->protocol,
@@ -465,6 +472,7 @@ pipex_unlink_session(struct pipex_sessio
{
session->ifindex = 0;
+ NET_ASSERT_LOCKED();
LIST_REMOVE(session, id_chain);
#if defined(PIPEX_PPTP) || defined(PIPEX_L2TP)
switch (session->protocol) {
