[PATCH] fix a race between /proc/lock_stat and module unloading

[Jerome Marchand]

When opening /proc/lock_stat, lock_stat_open() makes a copy of
all_lock_classes list in the form of an array of ad hoc structures
lock_stat_data that reference lock_class, so it can be sorted and
passed to seq_read(). However, nothing prevents module unloading code
to free some of these lock_class structures before seq_read() tries to
access them.

Copying the all lock_class structures instead of just their pointers
would be an easy fix, but it seems quite wasteful. This patch copies
only the needed data into the lock_stat_data structure.

Signed-off-by: Jerome Marchand <jmarchan@xxxxxxxxxx>
---
 kernel/locking/lockdep_proc.c | 88 ++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 38 deletions(-)

diff --git a/kernel/locking/lockdep_proc.c b/kernel/locking/lockdep_proc.c
index ef43ac4..c2eb8e8 100644
--- a/kernel/locking/lockdep_proc.c
+++ b/kernel/locking/lockdep_proc.c
@@ -363,7 +363,9 @@ static const struct file_operations proc_lockdep_stats_operations = {
 #ifdef CONFIG_LOCK_STAT
 
 struct lock_stat_data {
-	struct lock_class *class;
+	char name[39];
+	unsigned long contention_point[LOCKSTAT_POINTS];
+	unsigned long contending_point[LOCKSTAT_POINTS];
 	struct lock_class_stats stats;
 };
 
@@ -426,39 +428,12 @@ static void seq_lock_time(struct seq_file *m, struct lock_time *lt)
 
 static void seq_stats(struct seq_file *m, struct lock_stat_data *data)
 {
-	char name[39];
-	struct lock_class *class;
+	char *name = data->name;
 	struct lock_class_stats *stats;
-	int i, namelen;
+	int i, namelen = strlen(data->name);
 
-	class = data->class;
 	stats = &data->stats;
 
-	namelen = 38;
-	if (class->name_version > 1)
-		namelen -= 2; /* XXX truncates versions > 9 */
-	if (class->subclass)
-		namelen -= 2;
-
-	if (!class->name) {
-		char str[KSYM_NAME_LEN];
-		const char *key_name;
-
-		key_name = __get_key_name(class->key, str);
-		snprintf(name, namelen, "%s", key_name);
-	} else {
-		snprintf(name, namelen, "%s", class->name);
-	}
-	namelen = strlen(name);
-	if (class->name_version > 1) {
-		snprintf(name+namelen, 3, "#%d", class->name_version);
-		namelen += 2;
-	}
-	if (class->subclass) {
-		snprintf(name+namelen, 3, "/%d", class->subclass);
-		namelen += 2;
-	}
-
 	if (stats->write_holdtime.nr) {
 		if (stats->read_holdtime.nr)
 			seq_printf(m, "%38s-W:", name);
@@ -490,32 +465,32 @@ static void seq_stats(struct seq_file *m, struct lock_stat_data *data)
 	for (i = 0; i < LOCKSTAT_POINTS; i++) {
 		char ip[32];
 
-		if (class->contention_point[i] == 0)
+		if (data->contention_point[i] == 0)
 			break;
 
 		if (!i)
 			seq_line(m, '-', 40-namelen, namelen);
 
 		snprintf(ip, sizeof(ip), "[<%p>]",
-				(void *)class->contention_point[i]);
+				(void *)data->contention_point[i]);
 		seq_printf(m, "%40s %14lu %29s %pS\n",
 			   name, stats->contention_point[i],
-			   ip, (void *)class->contention_point[i]);
+			   ip, (void *)data->contention_point[i]);
 	}
 	for (i = 0; i < LOCKSTAT_POINTS; i++) {
 		char ip[32];
 
-		if (class->contending_point[i] == 0)
+		if (data->contending_point[i] == 0)
 			break;
 
 		if (!i)
 			seq_line(m, '-', 40-namelen, namelen);
 
 		snprintf(ip, sizeof(ip), "[<%p>]",
-				(void *)class->contending_point[i]);
+				(void *)data->contending_point[i]);
 		seq_printf(m, "%40s %14lu %29s %pS\n",
 			   name, stats->contending_point[i],
-			   ip, (void *)class->contending_point[i]);
+			   ip, (void *)data->contending_point[i]);
 	}
 	if (i) {
 		seq_puts(m, "\n");
@@ -593,6 +568,44 @@ static const struct seq_operations lockstat_ops = {
 	.show	= ls_show,
 };
 
+static void copy_lock_class(struct lock_stat_data *data,
+			    struct lock_class *class)
+{
+	char *name = data->name;
+	int namelen = 38;
+
+	if (class->name_version > 1)
+		namelen -= 2; /* XXX truncates versions > 9 */
+	if (class->subclass)
+		namelen -= 2;
+
+	if (!class->name) {
+		char str[KSYM_NAME_LEN];
+		const char *key_name;
+
+		key_name = __get_key_name(class->key, str);
+		snprintf(name, namelen, "%s", key_name);
+	} else {
+		snprintf(name, namelen, "%s", class->name);
+	}
+	namelen = strlen(name);
+	if (class->name_version > 1) {
+		snprintf(name+namelen, 3, "#%d", class->name_version);
+		namelen += 2;
+	}
+	if (class->subclass) {
+		snprintf(name+namelen, 3, "/%d", class->subclass);
+		namelen += 2;
+	}
+
+	memcpy(data->contention_point, class->contention_point,
+	       sizeof(class->contention_point));
+	memcpy(data->contending_point, class->contending_point,
+	       sizeof(class->contending_point));
+
+	data->stats = lock_stats(class);
+}
+
 static int lock_stat_open(struct inode *inode, struct file *file)
 {
 	int res;
@@ -608,8 +621,7 @@ static int lock_stat_open(struct inode *inode, struct file *file)
 		struct seq_file *m = file->private_data;
 
 		list_for_each_entry(class, &all_lock_classes, lock_entry) {
-			iter->class = class;
-			iter->stats = lock_stats(class);
+			copy_lock_class(iter, class);
 			iter++;
 		}
 		data->iter_end = iter;
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/