[PATCH] sg.c: fix a memory leak in devices seq_file implementation

Jan Blunck · Linus Torvalds · commit 729d70f5dfd6 · 2005-08-27T11:22:27.000-07:00
I know that scsi procfs is legacy code but this is a fix for a memory leak.

While reading through sg.c I realized that the implementation of
/proc/scsi/sg/devices with seq_file is leaking memory due to freeing the
pointer returned by the next() iterator method.  Since next() might return
NULL or an error this is wrong.  This patch fixes it through using the
seq_files private field for holding the reference to the iterator object.

Here is a small bash script to trigger the leak. Use slabtop to watch
the size-32 usage grow and grow.

#!/bin/sh

while true; do
	cat /proc/scsi/sg/devices &gt; /dev/null
done

Signed-off-by: Jan Blunck &lt;j.blunck@tu-harburg.de&gt;
Acked-by: James Bottomley &lt;James.Bottomley@steeleye.com&gt;
Signed-off-by: Andrew Morton &lt;akpm@osdl.org&gt;
Signed-off-by: Linus Torvalds &lt;torvalds@osdl.org&gt;
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
@@ -2971,31 +2971,30 @@ static void * dev_seq_start(struct seq_file *s, loff_t *pos)
 {
 	struct sg_proc_deviter * it = kmalloc(sizeof(*it), GFP_KERNEL);
 
+	s->private = it;
 	if (! it)
 		return NULL;
+
 	if (NULL == sg_dev_arr)
-		goto err1;
+		return NULL;
 	it->index = *pos;
 	it->max = sg_last_dev();
 	if (it->index >= it->max)
-		goto err1;
+		return NULL;
 	return it;
-err1:
-	kfree(it);
-	return NULL;
 }
 
 static void * dev_seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
-	struct sg_proc_deviter * it = (struct sg_proc_deviter *) v;
+	struct sg_proc_deviter * it = s->private;
 
 	*pos = ++it->index;
 	return (it->index < it->max) ? it : NULL;
 }
 
 static void dev_seq_stop(struct seq_file *s, void *v)
 {
-	kfree (v);
+	kfree(s->private);
 }
 
 static int sg_proc_open_dev(struct inode *inode, struct file *file)

Original file line number	Diff line number	Diff line change
`@@ -2971,31 +2971,30 @@ static void * dev_seq_start(struct seq_file s, loff_t pos)`
`2971`	`2971`	`{`
`2972`	`2972`	`struct sg_proc_deviter * it = kmalloc(sizeof(*it), GFP_KERNEL);`
`2973`	`2973`
	`2974`	`+ s->private = it;`
`2974`	`2975`	`if (! it)`
`2975`	`2976`	`return NULL;`
	`2977`	`+`
`2976`	`2978`	`if (NULL == sg_dev_arr)`
`2977`		`- goto err1;`
	`2979`	`+ return NULL;`
`2978`	`2980`	`it->index = *pos;`
`2979`	`2981`	`it->max = sg_last_dev();`
`2980`	`2982`	`if (it->index >= it->max)`
`2981`		`- goto err1;`
	`2983`	`+ return NULL;`
`2982`	`2984`	`return it;`
`2983`		`-err1:`
`2984`		`- kfree(it);`
`2985`		`- return NULL;`
`2986`	`2985`	`}`
`2987`	`2986`
`2988`	`2987`	`static void * dev_seq_next(struct seq_file s, void v, loff_t *pos)`
`2989`	`2988`	`{`
`2990`		`- struct sg_proc_deviter * it = (struct sg_proc_deviter *) v;`
	`2989`	`+ struct sg_proc_deviter * it = s->private;`
`2991`	`2990`
`2992`	`2991`	`*pos = ++it->index;`
`2993`	`2992`	`return (it->index < it->max) ? it : NULL;`
`2994`	`2993`	`}`
`2995`	`2994`
`2996`	`2995`	`static void dev_seq_stop(struct seq_file s, void v)`
`2997`	`2996`	`{`
`2998`		`- kfree (v);`
	`2997`	`+ kfree(s->private);`
`2999`	`2998`	`}`
`3000`	`2999`
`3001`	`3000`	`static int sg_proc_open_dev(struct inode inode, struct file file)`