linux-kernel - Re: [PATCH v3] dma-debug: print some unfreed allocations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 May 2011 14:35:08 +0200
From:	"Roedel, Joerg" <Joerg.Roedel@....com>
To:	Johannes Berg <johannes@...solutions.net>
CC:	linux-kernel <linux-kernel@...r.kernel.org>,
	David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH v3] dma-debug: print some unfreed allocations

On Fri, May 20, 2011 at 12:36:58PM -0400, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@...el.com>
>  #define NAME_MAX_LEN	64
> @@ -641,6 +647,14 @@ static int dma_debug_fs_init(void)
>  	if (!filter_dent)
>  		goto out_err;
>  
> +#ifdef CONFIG_STACKTRACE
> +	num_show_pending_dent = debugfs_create_u32("num_show_pending", 0644,
> +						   dma_debug_dent,
> +						   &num_show_pending);
> +	if (!num_show_pending_dent)
> +		goto out_err;
> +#endif

Hmm, thinking more about this, do we need to introduce a new variable at
all? It should fit well into the dma-api/all_errors and
dma-api/num_errors configurables. 

> +#ifdef CONFIG_STACKTRACE
> +	if (count > 1 && num_show_pending > 0) {
> +		count = 0;
> +		/*
> +		 * If we have, print out some stack traces for the allocations.
> +		 * In case of module unload, the stack traces will be useless,
> +		 * but instead of unloading the module you can manually unbind
> +		 * the driver instead and get useful traces.
> +		 */
> +		printk(KERN_WARNING "Showing traces for %d allocations:\n",
> +		       num_show_pending);
> +
> +		for (i = 0; i < HASH_SIZE; ++i) {
> +			spin_lock(&dma_entry_hash[i].lock);
> +			list_for_each_entry(entry, &dma_entry_hash[i].list,
> +					    list) {
> +				if (entry->dev != dev)
> +					continue;
> +				count += 1;
> +				if (count > num_show_pending)
> +					break;
> +				dump_entry_trace(entry);
> +			}
> +			spin_unlock(&dma_entry_hash[i].lock);
> +
> +			if (count > num_show_pending)
> +				break;
> +		}
> +	}
> +#endif

This duplicates the loop above. The prints can be folded into one loop.
How about doing it this way (patch is only compile-tested):

diff --git a/lib/dma-debug.c b/lib/dma-debug.c
index db07bfd..ee21ef0 100644
--- a/lib/dma-debug.c
+++ b/lib/dma-debug.c
@@ -187,11 +187,14 @@ static bool driver_filter(struct device *dev)
 	return ret;
 }
 
-#define err_printk(dev, entry, format, arg...) do {			\
+#define err_warn(format, arg...)		WARN(1, format, ## arg)
+#define err_no_warn(format, arg...)		pr_err(format, ## arg)
+
+#define __err_printk(func, dev, entry, format, arg...) do {		\
 		error_count += 1;					\
 		if (driver_filter(dev) &&				\
 		    (show_all_errors || show_num_errors > 0)) {		\
-			WARN(1, "%s %s: " format,			\
+			func("%s %s: " format,				\
 			     dev ? dev_driver_string(dev) : "NULL",	\
 			     dev ? dev_name(dev) : "NULL", ## arg);	\
 			dump_entry_trace(entry);			\
@@ -200,6 +203,12 @@ static bool driver_filter(struct device *dev)
 			show_num_errors -= 1;				\
 	} while (0);
 
+#define err_printk(dev, entry, format, arg...)				\
+	__err_printk(err_warn, dev, entry, format, ## arg)
+
+#define err_printk_no_warn(dev, entry, format, arg...)			\
+	__err_printk(err_no_warn, dev, entry, format, ## arg)
+
 /*
  * Hash related functions
  *
@@ -649,52 +658,38 @@ out_err:
 	return -ENOMEM;
 }
 
-static int device_dma_allocations(struct device *dev, struct dma_debug_entry **out_entry)
+static void check_device_dma_allocations(struct device *dev)
 {
 	struct dma_debug_entry *entry;
 	unsigned long flags;
-	int count = 0, i;
+	int i;
 
 	local_irq_save(flags);
 
 	for (i = 0; i < HASH_SIZE; ++i) {
 		spin_lock(&dma_entry_hash[i].lock);
 		list_for_each_entry(entry, &dma_entry_hash[i].list, list) {
-			if (entry->dev == dev) {
-				count += 1;
-				*out_entry = entry;
-			}
+			if (entry->dev != dev)
+				continue;
+			err_printk_no_warn(dev, entry,
+				"DMA-API: device driver has pending DMA allocation\n");
 		}
 		spin_unlock(&dma_entry_hash[i].lock);
 	}
 
 	local_irq_restore(flags);
-
-	return count;
 }
 
 static int dma_debug_device_change(struct notifier_block *nb, unsigned long action, void *data)
 {
 	struct device *dev = data;
-	struct dma_debug_entry *uninitialized_var(entry);
-	int count;
 
 	if (global_disable)
 		return 0;
 
 	switch (action) {
 	case BUS_NOTIFY_UNBOUND_DRIVER:
-		count = device_dma_allocations(dev, &entry);
-		if (count == 0)
-			break;
-		err_printk(dev, entry, "DMA-API: device driver has pending "
-				"DMA allocations while released from device "
-				"[count=%d]\n"
-				"One of leaked entries details: "
-				"[device address=0x%016llx] [size=%llu bytes] "
-				"[mapped with %s] [mapped as %s]\n",
-			count, entry->dev_addr, entry->size,
-			dir2name[entry->direction], type2name[entry->type]);
+		check_device_dma_allocations(dev);
 		break;
 	default:
 		break;

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