Skip to contentSkip to conversation

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

PATCH In review Jan Blunck 1 participant 2 messages Aug 23, 2005 1 message not in the archive

Subsystems (2)
SCSI SCSI SG driver
lore
Share & tools
lore

Jan Blunck

author
#0 lore raw

PATCH 1/1 sg.c: fix a memory leak in devices seq_file implementation

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.

[--snipp--]
#!/bin/sh

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

[--snipp--]

Signed-off-by Jan Blunck <[email protected]>
 drivers/scsi/sg.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)
drivers/scsi/sg.c +9 -7
Index: experimental-jb/drivers/scsi/sg.c
===================================================================
@@ -2971,23 +2971,22 @@ static void * dev_seq_start(struct seq_f
 {
 	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;

@@ -2995,7 +2994,10 @@ static void * dev_seq_next(struct seq_fi
 
 static void dev_seq_stop(struct seq_file *s, void *v)
 {
-	kfree (v);
+	struct sg_proc_deviter *it = s->private;
+
+	if (it)
+		kfree (it);
 }
 
 static int sg_proc_open_dev(struct inode *inode, struct file *file)

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw

Re: [PATCH] sg.c: fix a memory leak in devices seq_file implementation (2nd)

Quoted 10 lines · Ingo Oeser

kfree() checks its argument, so instead of

if (pointer)

	kfree(pointer);

just do

kfree(pointer);

This is an updated patch. Removed the unnecessary if.

Signed-off-by Jan Blunck <[email protected]>
 drivers/scsi/sg.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
drivers/scsi/sg.c +8 -7
Index: linux-2.6/drivers/scsi/sg.c
===================================================================
@@ -2971,23 +2971,22 @@ static void * dev_seq_start(struct seq_f
 {
 	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;

@@ -2995,7 +2994,9 @@ static void * dev_seq_next(struct seq_fi
 
 static void dev_seq_stop(struct seq_file *s, void *v)
 {
-	kfree (v);
+	struct sg_proc_deviter * it = s->private;
+
+	kfree (it);
 }
 
 static int sg_proc_open_dev(struct inode *inode, struct file *file)