netfilter: xt_qtaguid: fix crash after using delete ctrl command

* Crash fix
The delete command would delete a socket tag entry without removing it
from the proc_qtu_data { ..., sock_tag_list, }.
This in turn would cause an exiting process to crash while cleaning up
its matching proc_qtu_data.

* Added more aggressive tracking/cleanup of proc_qtu_data
This should allow one process to cleanup qtu_tag_data{} left around from
processes that didn't use resource tracking via /dev/xt_qtaguid.

* Debug printing tweaks
Better code inclusion/exclusion handling,
and extra debug out of full state.

Change-Id: I735965af2962ffcd7f3021cdc0068b3ab21245c2
Signed-off-by: JP Abgrall <jpa@google.com>

1 files changed, 158 insertions, 101 deletions

diff --git a/net/netfilter/xt_qtaguid.c b/net/netfilter/xt_qtaguid.c
index c42e486c5cbe..32d855b1b6d2 100644
--- a/net/netfilter/xt_qtaguid.c
+++ b/net/netfilter/xt_qtaguid.c
@@ -102,11 +102,10 @@ static bool qtu_proc_handling_passive;
 module_param_named(tag_tracking_passive, qtu_proc_handling_passive, bool,
 		   S_IRUGO | S_IWUSR);
 
-
 #define QTU_DEV_NAME "xt_qtaguid"
 
-uint debug_mask = DEFAULT_DEBUG_MASK;
-module_param(debug_mask, uint, S_IRUGO | S_IWUSR);
+uint qtaguid_debug_mask = DEFAULT_DEBUG_MASK;
+module_param_named(debug_mask, qtaguid_debug_mask, uint, S_IRUGO | S_IWUSR);
 
 /*---------------------------------------------------------------------------*/
 static const char *iface_stat_procdirname = "iface_stat";
@@ -125,70 +124,92 @@ static struct proc_dir_entry *iface_stat_all_procfile;
  * Notice how sock_tag_list_lock is held sometimes when uid_tag_data_tree_lock
  * is acquired.
  *
- * Call tree with all lock holders as of 2011-09-06:
- *
- *   qtaguid_ctrl_parse()
- *     ctrl_cmd_delete()
- *       sock_tag_list_lock
- *       tag_counter_set_list_lock
- *       iface_stat_list_lock
- *         iface_entry->tag_stat_list_lock
- *       uid_tag_data_tree_lock
- *     ctrl_cmd_counter_set()
- *       tag_counter_set_list_lock
- *     ctrl_cmd_tag()
- *       sock_tag_list_lock
- *         get_tag_ref()
- *           uid_tag_data_tree_lock
- *       uid_tag_data_tree_lock
- *     ctrl_cmd_untag()
- *       sock_tag_list_lock
- *         uid_tag_data_tree_lock
+ * Call tree with all lock holders as of 2011-09-25:
  *
- *   qtaguid_mt()
- *     account_for_uid()
- *       if_tag_stat_update()
- *     get_sock_stat()
- *       sock_tag_list_lock
- *        iface_entry->tag_stat_list_lock
- *        tag_stat_update()
- *          get_active_counter_set()
- *            tag_counter_set_list_lock
+ * iface_stat_all_proc_read()
+ *   iface_stat_list_lock
+ *     (struct iface_stat)
  *
- *   iface_netdev_event_handler()
- *     iface_stat_create()
- *       iface_stat_list_lock
- *     iface_stat_update()
- *       iface_stat_list_lock
+ * qtaguid_ctrl_proc_read()
+ *   sock_tag_list_lock
+ *     (sock_tag_tree)
+ *     (struct proc_qtu_data->sock_tag_list)
+ *   prdebug_full_state()
+ *     sock_tag_list_lock
+ *       (sock_tag_tree)
+ *     uid_tag_data_tree_lock
+ *       (uid_tag_data_tree)
+ *       (proc_qtu_data_tree)
+ *     iface_stat_list_lock
  *
- *   iface_inet6addr_event_handler()
- *     iface_stat_create_ipv6()
- *       iface_stat_list_lock
- *     iface_stat_update()
- *       iface_stat_list_lock
+ * qtaguid_stats_proc_read()
+ *   iface_stat_list_lock
+ *     struct iface_stat->tag_stat_list_lock
  *
- *   iface_inetaddr_event_handler()
- *     iface_stat_create()
- *       iface_stat_list_lock
- *     iface_stat_update()
- *       iface_stat_list_lock
+ * qtudev_open()
+ *   uid_tag_data_tree_lock
  *
- *   qtaguid_ctrl_proc_read()
- *     sock_tag_list_lock
+ * qtudev_release()
+ *   sock_tag_data_list_lock
+ *     uid_tag_data_tree_lock
+ *   prdebug_full_state()
  *     sock_tag_list_lock
  *     uid_tag_data_tree_lock
  *     iface_stat_list_lock
  *
- *   qtaguid_stats_proc_read()
+ * iface_netdev_event_handler()
+ *   iface_stat_create()
+ *     iface_stat_list_lock
+ *   iface_stat_update()
  *     iface_stat_list_lock
- *       iface_entry->tag_stat_list_lock
  *
- *   qtudev_open()
- *     uid_tag_data_tree_lock
+ * iface_inetaddr_event_handler()
+ *   iface_stat_create()
+ *     iface_stat_list_lock
+ *   iface_stat_update()
+ *     iface_stat_list_lock
+ *
+ * iface_inet6addr_event_handler()
+ *   iface_stat_create_ipv6()
+ *     iface_stat_list_lock
+ *   iface_stat_update()
+ *     iface_stat_list_lock
  *
- *   qtud_dev_release()
+ * qtaguid_mt()
+ *   account_for_uid()
+ *     if_tag_stat_update()
+ *       get_sock_stat()
+ *         sock_tag_list_lock
+ *       struct iface_stat->tag_stat_list_lock
+ *         tag_stat_update()
+ *           get_active_counter_set()
+ *             tag_counter_set_list_lock
+ *         tag_stat_update()
+ *           get_active_counter_set()
+ *             tag_counter_set_list_lock
+ *
+ *
+ * qtaguid_ctrl_parse()
+ *   ctrl_cmd_delete()
+ *     sock_tag_list_lock
+ *     tag_counter_set_list_lock
+ *     iface_stat_list_lock
+ *       struct iface_stat->tag_stat_list_lock
+ *     uid_tag_data_tree_lock
+ *   ctrl_cmd_counter_set()
+ *     tag_counter_set_list_lock
+ *   ctrl_cmd_tag()
  *     sock_tag_list_lock
+ *       (sock_tag_tree)
+ *       get_tag_ref()
+ *         uid_tag_data_tree_lock
+ *           (uid_tag_data_tree)
  *       uid_tag_data_tree_lock
+ *         (proc_qtu_data_tree)
+ *   ctrl_cmd_untag()
+ *     sock_tag_list_lock
+ *     uid_tag_data_tree_lock
+ *
  */
 static LIST_HEAD(iface_stat_list);
 static DEFINE_SPINLOCK(iface_stat_list_lock);
@@ -557,8 +578,8 @@ static struct tag_ref *new_tag_ref(tag_t new_tag,
 	utd_entry->num_active_tags++;
 	tag_ref_tree_insert(tr_entry, &utd_entry->tag_ref_tree);
 	DR_DEBUG("qtaguid: new_tag_ref(0x%llx): "
-		 " inserted new tag ref\n",
-		 new_tag);
+		 " inserted new tag ref %p\n",
+		 new_tag, tr_entry);
 	return tr_entry;
 
 err_res:
@@ -618,7 +639,8 @@ static struct tag_ref *get_tag_ref(tag_t full_tag,
 static void put_utd_entry(struct uid_tag_data *utd_entry)
 {
 	/* Are we done with the UID tag data entry? */
-	if (RB_EMPTY_ROOT(&utd_entry->tag_ref_tree)) {
+	if (RB_EMPTY_ROOT(&utd_entry->tag_ref_tree) &&
+		!utd_entry->num_pqd) {
 		DR_DEBUG("qtaguid: %s(): "
 			 "erase utd_entry=%p uid=%u "
 			 "by pid=%u tgid=%u uid=%u\n", __func__,
@@ -629,9 +651,11 @@ static void put_utd_entry(struct uid_tag_data *utd_entry)
 		kfree(utd_entry);
 	} else {
 		DR_DEBUG("qtaguid: %s(): "
-			 "utd_entry=%p still has %d tags\n", __func__,
-			 utd_entry, utd_entry->num_active_tags);
-		BUG_ON(!utd_entry->num_active_tags);
+			 "utd_entry=%p still has %d tags %d proc_qtu_data\n",
+			 __func__, utd_entry, utd_entry->num_active_tags,
+			 utd_entry->num_pqd);
+		BUG_ON(!(utd_entry->num_active_tags ||
+			 utd_entry->num_pqd));
 	}
 }
 
@@ -1309,8 +1333,8 @@ static void if_tag_stat_update(const char *ifname, uid_t uid,
 		new_tag_stat = create_if_tag_stat(iface_entry, tag);
 		new_tag_stat->parent_counters = uid_tag_counters;
 	}
-	spin_unlock_bh(&iface_entry->tag_stat_list_lock);
 	tag_stat_update(new_tag_stat, direction, proto, bytes);
+	spin_unlock_bh(&iface_entry->tag_stat_list_lock);
 }
 
 static int iface_netdev_event_handler(struct notifier_block *nb,
@@ -1672,6 +1696,50 @@ ret_res:
 	return res;
 }
 
+#ifdef DDEBUG
+/* This function is not in xt_qtaguid_print.c because of locks visibility */
+static void prdebug_full_state(int indent_level, const char *fmt, ...)
+{
+	va_list args;
+	char *fmt_buff;
+	char *buff;
+
+	if (!unlikely(qtaguid_debug_mask & DDEBUG_MASK))
+		return;
+
+	fmt_buff = kasprintf(GFP_ATOMIC,
+			     "qtaguid: %s(): %s {\n", __func__, fmt);
+	BUG_ON(!fmt_buff);
+	va_start(args, fmt);
+	buff = kvasprintf(GFP_ATOMIC,
+			  fmt_buff, args);
+	BUG_ON(!buff);
+	pr_debug("%s", buff);
+	kfree(fmt_buff);
+	kfree(buff);
+	va_end(args);
+
+	spin_lock_bh(&sock_tag_list_lock);
+	prdebug_sock_tag_tree(indent_level, &sock_tag_tree);
+	spin_unlock_bh(&sock_tag_list_lock);
+
+	spin_lock_bh(&sock_tag_list_lock);
+	spin_lock_bh(&uid_tag_data_tree_lock);
+	prdebug_uid_tag_data_tree(indent_level, &uid_tag_data_tree);
+	prdebug_proc_qtu_data_tree(indent_level, &proc_qtu_data_tree);
+	spin_unlock_bh(&uid_tag_data_tree_lock);
+	spin_unlock_bh(&sock_tag_list_lock);
+
+	spin_lock_bh(&iface_stat_list_lock);
+	prdebug_iface_stat_list(indent_level, &iface_stat_list);
+	spin_unlock_bh(&iface_stat_list_lock);
+
+	pr_debug("qtaguid: %s(): }\n", __func__);
+}
+#else
+static void prdebug_full_state(int indent_level, const char *fmt, ...) {}
+#endif
+
 /*
  * Procfs reader to get all active socket tags using style "1)" as described in
  * fs/proc/generic.c
@@ -1761,28 +1829,10 @@ static int qtaguid_ctrl_proc_read(char *page, char **num_items_returned,
 		(*num_items_returned)++;
 	}
 
-#ifdef CDEBUG
 	/* Count the following as part of the last item_index */
 	if (item_index > items_to_skip) {
-		CT_DEBUG("qtaguid: proc ctrl state debug {\n");
-		spin_lock_bh(&sock_tag_list_lock);
-		prdebug_sock_tag_tree(indent_level, &sock_tag_tree);
-		spin_unlock_bh(&sock_tag_list_lock);
-
-		spin_lock_bh(&uid_tag_data_tree_lock);
-		prdebug_uid_tag_data_tree(indent_level, &uid_tag_data_tree);
-		prdebug_proc_qtu_data_tree(indent_level, &proc_qtu_data_tree);
-		spin_unlock_bh(&uid_tag_data_tree_lock);
-
-		spin_lock_bh(&iface_stat_list_lock);
-		prdebug_iface_stat_list(indent_level, &iface_stat_list);
-		spin_unlock_bh(&iface_stat_list_lock);
-
-		CT_DEBUG("qtaguid: proc ctrl state debug }\n");
-
-
+		prdebug_full_state(indent_level, "proc ctrl");
 	}
-#endif
 
 	*eof = 1;
 	return outp - page;
@@ -1833,9 +1883,9 @@ static int ctrl_cmd_delete(const char *input)
 	}
 
 	tag = combine_atag_with_uid(acct_tag, uid);
-	CT_DEBUG("qtaguid: ctrl_delete(): "
+	CT_DEBUG("qtaguid: ctrl_delete(%s): "
 		 "looking for tag=0x%llx (uid=%u)\n",
-		 tag, uid);
+		 input, tag, uid);
 
 	/* Delete socket tags */
 	spin_lock_bh(&sock_tag_list_lock);
@@ -1847,8 +1897,8 @@ static int ctrl_cmd_delete(const char *input)
 		if (entry_uid != uid)
 			continue;
 
-		CT_DEBUG("qtaguid: ctrl_delete(): st tag=0x%llx (uid=%u)\n",
-			 st_entry->tag, entry_uid);
+		CT_DEBUG("qtaguid: ctrl_delete(%s): st tag=0x%llx (uid=%u)\n",
+			 input, st_entry->tag, entry_uid);
 
 		if (!acct_tag || st_entry->tag == tag) {
 			rb_erase(&st_entry->sock_node, &sock_tag_tree);
@@ -1857,6 +1907,7 @@ static int ctrl_cmd_delete(const char *input)
 			tr_entry = lookup_tag_ref(st_entry->tag, NULL);
 			BUG_ON(tr_entry->num_sock_tags <= 0);
 			tr_entry->num_sock_tags--;
+			list_del(&st_entry->list);
 		}
 	}
 	spin_unlock_bh(&sock_tag_list_lock);
@@ -1868,8 +1919,9 @@ static int ctrl_cmd_delete(const char *input)
 	/* Counter sets are only on the uid tag, not full tag */
 	tcs_entry = tag_counter_set_tree_search(&tag_counter_set_tree, tag);
 	if (tcs_entry) {
-		CT_DEBUG("qtaguid: ctrl_delete(): "
+		CT_DEBUG("qtaguid: ctrl_delete(%s): "
 			 "erase tcs: tag=0x%llx (uid=%u) set=%d\n",
+			 input,
 			 tcs_entry->tn.tag,
 			 get_uid_from_tag(tcs_entry->tn.tag),
 			 tcs_entry->active_set);
@@ -1891,16 +1943,16 @@ static int ctrl_cmd_delete(const char *input)
 			entry_uid = get_uid_from_tag(ts_entry->tn.tag);
 			node = rb_next(node);
 
-			CT_DEBUG("qtaguid: ctrl_delete(): "
+			CT_DEBUG("qtaguid: ctrl_delete(%s): "
 				 "ts tag=0x%llx (uid=%u)\n",
-				 ts_entry->tn.tag, entry_uid);
+				 input, ts_entry->tn.tag, entry_uid);
 
 			if (entry_uid != uid)
 				continue;
 			if (!acct_tag || ts_entry->tn.tag == tag) {
-				CT_DEBUG("qtaguid: ctrl_delete(): "
+				CT_DEBUG("qtaguid: ctrl_delete(%s): "
 					 "erase ts: %s 0x%llx %u\n",
-					 iface_entry->ifname,
+					 input, iface_entry->ifname,
 					 get_atag_from_tag(ts_entry->tn.tag),
 					 entry_uid);
 				rb_erase(&ts_entry->tn.node,
@@ -1920,9 +1972,9 @@ static int ctrl_cmd_delete(const char *input)
 		entry_uid = utd_entry->uid;
 		node = rb_next(node);
 
-		CT_DEBUG("qtaguid: ctrl_delete(): "
+		CT_DEBUG("qtaguid: ctrl_delete(%s): "
 			 "utd uid=%u\n",
-			 entry_uid);
+			 input, entry_uid);
 
 		if (entry_uid != uid)
 			continue;
@@ -2377,7 +2429,7 @@ static int pp_stats_line(struct proc_print_info *ppi, int cnt_set)
 	return len;
 }
 
-bool pp_sets(struct proc_print_info *ppi)
+static bool pp_sets(struct proc_print_info *ppi)
 {
 	int len;
 	int counter_set;
@@ -2472,7 +2524,7 @@ static int qtudev_open(struct inode *inode, struct file *file)
 {
 	struct uid_tag_data *utd_entry;
 	struct proc_qtu_data  *pqd_entry;
-	struct proc_qtu_data  *new_pqd_entry = 0;
+	struct proc_qtu_data  *new_pqd_entry;
 	int res;
 	bool utd_entry_found;
 
@@ -2514,12 +2566,14 @@ static int qtudev_open(struct inode *inode, struct file *file)
 	new_pqd_entry->pid = current->tgid;
 	INIT_LIST_HEAD(&new_pqd_entry->sock_tag_list);
 	new_pqd_entry->parent_tag_data = utd_entry;
+	utd_entry->num_pqd++;
 
 	proc_qtu_data_tree_insert(new_pqd_entry,
 				  &proc_qtu_data_tree);
 
 	spin_unlock_bh(&uid_tag_data_tree_lock);
-	DR_DEBUG("qtaguid: tracking data for uid=%u\n", current_fsuid());
+	DR_DEBUG("qtaguid: tracking data for uid=%u in pqd=%p\n",
+		 current_fsuid(), new_pqd_entry);
 	file->private_data = new_pqd_entry;
 	return 0;
 
@@ -2559,12 +2613,6 @@ static int qtudev_release(struct inode *inode, struct file *file)
 	spin_lock_bh(&sock_tag_list_lock);
 	spin_lock_bh(&uid_tag_data_tree_lock);
 
-	/*
-	 * If this proc didn't actually tag anything for itself, or has already
-	 * willingly cleaned up itself ...
-	 */
-	put_utd_entry(utd_entry);
-
 	list_for_each_safe(entry, next, &pqd_entry->sock_tag_list) {
 		st_entry = list_entry(entry, struct sock_tag, list);
 		DR_DEBUG("qtaguid: %s(): "
@@ -2593,10 +2641,18 @@ static int qtudev_release(struct inode *inode, struct file *file)
 		/* Can't sockfd_put() within spinlock, do it later. */
 		sock_tag_tree_insert(st_entry, &st_to_free_tree);
 
-		/* Do not put_utd_entry(utd_entry) someone elses utd_entry */
+		/*
+		 * Try to free the utd_entry if no other proc_qtu_data is
+		 * using it (num_pqd is 0) and it doesn't have active tags
+		 * (num_active_tags is 0).
+		 */
+		put_utd_entry(utd_entry);
 	}
 
 	rb_erase(&pqd_entry->node, &proc_qtu_data_tree);
+	BUG_ON(pqd_entry->parent_tag_data->num_pqd < 1);
+	pqd_entry->parent_tag_data->num_pqd--;
+	put_utd_entry(pqd_entry->parent_tag_data);
 	kfree(pqd_entry);
 	file->private_data = NULL;
 
@@ -2606,7 +2662,8 @@ static int qtudev_release(struct inode *inode, struct file *file)
 
 	sock_tag_tree_erase(&st_to_free_tree);
 
-
+	prdebug_full_state(0, "%s(): pid=%u tgid=%u", __func__,
+			   current->pid, current->tgid);
 	return 0;
 }