On Fri, Jul 10, 2020 at 10:54:44AM +0200, Martin Pieuchot wrote:
> On 07/07/20(Tue) 01:01, Vitaliy Makkoveev wrote:
> > On Mon, Jul 06, 2020 at 08:47:23PM +0200, Martin Pieuchot wrote:
> > > On 06/07/20(Mon) 19:23, Vitaliy Makkoveev wrote:
> > > > > On 6 Jul 2020, at 17:36, Martin Pieuchot <[email protected]> wrote:
> > > > [...]
> > > > Unfortunately you can’t be sure about NET_LOCK() status while you are
> > > > in pppac_start(). It was described at this thread [1].
> > > >
> > > > We have two cases:
> > > > 1. pppac_start() called from pppac_output(). NET_LOCK() was inherited.
> > >
> > > Such recursions should be avoided. if_enqueue() should take care of
> > > that.
> >
> > I suggest to finish the route to if_get(9) before. Updated diff which
> > removes pipexintr() below. Just against the most resent source tree.
>
> The tasks are not orthogonal. Making sure the NET_LOCK() is taken
> inside the pipex boundaries help for this task as well.
>
> That said the current code is not ready for the proposed diff. At
> least `pppx_devs', `pipex_rd_head4' and `pipex_rd_head6' must be
> protected/annotated.
>
> What about all the lists/hashtables? They aren't annotated, are they
> all protected by the NET_LOCK()?
>
> What about `pppx_ifs' is it only used under the KERNEL_LOCK()?
Ok, let's document pipex(4) globals first.
There is no reason to do extra protection to ppp{ac,x}(4) globals. We
*always* destroy pipex(4) session *before* corresponding ppp{ac,x}(4)
context. And ppp{ac,x}(4) layer is the only place where we create and
destroy sessions. KERNEL_LOCK() does the protection for this layer.
Index: sys/net/pipex.c
===================================================================
RCS file: /cvs/src/sys/net/pipex.c,v
retrieving revision 1.119
diff -u -p -r1.119 pipex.c
--- sys/net/pipex.c 6 Jul 2020 20:37:51 -0000 1.119
+++ sys/net/pipex.c 11 Jul 2020 13:09:40 -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:
+ * I immutable after creation
+ * A atomic operation
+ * 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. */
