pipex(4): document global data locks

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

pipex(4): document global data locks

Vitaliy Makkoveev-2
Subj. Also add NET_ASSERT_LOCKED() to pipex_{link,unlink,rele}_session()
to be sure they called under NET_LOCK().

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 17 Jul 2020 14:01:10 -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. */
@@ -419,6 +424,8 @@ pipex_init_session(struct pipex_session
 void
 pipex_rele_session(struct pipex_session *session)
 {
+ NET_ASSERT_LOCKED();
+
  if (session->mppe_recv.old_session_keys)
  pool_put(&mppe_key_pool, session->mppe_recv.old_session_keys);
  pool_put(&pipex_session_pool, session);
@@ -430,6 +437,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,
@@ -463,6 +472,8 @@ pipex_link_session(struct pipex_session
 void
 pipex_unlink_session(struct pipex_session *session)
 {
+ NET_ASSERT_LOCKED();
+
  session->ifindex = 0;
 
  LIST_REMOVE(session, id_chain);

Reply | Threaded
Open this post in threaded view
|

Re: pipex(4): document global data locks

Martin Pieuchot
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.

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.

> 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 17 Jul 2020 14:01:10 -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. */
> @@ -419,6 +424,8 @@ pipex_init_session(struct pipex_session
>  void
>  pipex_rele_session(struct pipex_session *session)
>  {
> + NET_ASSERT_LOCKED();
> +
>   if (session->mppe_recv.old_session_keys)
>   pool_put(&mppe_key_pool, session->mppe_recv.old_session_keys);
>   pool_put(&pipex_session_pool, session);
> @@ -430,6 +437,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,
> @@ -463,6 +472,8 @@ pipex_link_session(struct pipex_session
>  void
>  pipex_unlink_session(struct pipex_session *session)
>  {
> + NET_ASSERT_LOCKED();
> +
>   session->ifindex = 0;
>  
>   LIST_REMOVE(session, id_chain);
>

Reply | Threaded
Open this post in threaded view
|

Re: pipex(4): document global data locks

Vitaliy Makkoveev-2
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) {