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);
>
