stm class: fix device leak in open error path

PATCH In review Johan Hovold 2 participants 4 messages Nov 1, 2016 – Nov 18, 2016

Subsystems (3)

Arm STM32 Hardware tracing facilities System trace module class

lore

Share & tools

lore

Johan Hovold

author

2016-11-01 10:45 UTC #0 lore raw

PATCH 1/1 stm class: fix device leak in open error path

Make sure to drop the reference taken by class_find_device() also on allocation errors in open().

Fixes 7bd1d4093c2f ("stm class: Introduce an abstraction for...")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/hwtracing/stm/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/hwtracing/stm/core.c +6 -3

index 51f81d64ca37..47811f5c9bc9 100644
@@ -368,8 +368,10 @@ static int stm_char_open(struct inode *inode, struct file *file)
 		return -ENODEV;
 
 	stmf = kzalloc(sizeof(*stmf), GFP_KERNEL);
-	if (!stmf)
-		return -ENOMEM;
+	if (!stmf) {
+		err = -ENOMEM;
+		goto err_put_device;
+	}
 
 	stm_output_init(&stmf->output);
 	stmf->stm = to_stm_device(dev);

@@ -382,9 +384,10 @@ static int stm_char_open(struct inode *inode, struct file *file)
 	return nonseekable_open(inode, file);
 
 err_free:
+	kfree(stmf);
+err_put_device:
 	/* matches class_find_device() above */
 	put_device(dev);
-	kfree(stmf);
 
 	return err;
 }

Signature · 1 line

2.7.3

Johan Hovold

author

2016-11-17 18:34 UTC #1 lore raw

Quoted 28 lines · Johan Hovold

Make sure to drop the reference taken by class_find_device() also on allocation errors in open().

Fixes 7bd1d4093c2f ("stm class: Introduce an abstraction for...")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/hwtracing/stm/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/hwtracing/stm/core.c +4 -2

index 51f81d64ca37..47811f5c9bc9 100644
@@ -368,8 +368,10 @@ static int stm_char_open(struct inode *inode, struct file *file)
 		return -ENODEV;
 
 	stmf = kzalloc(sizeof(*stmf), GFP_KERNEL);
-	if (!stmf)
-		return -ENOMEM;
+	if (!stmf) {
+		err = -ENOMEM;
+		goto err_put_device;
+	}
 
 	stm_output_init(&stmf->output);
 	stmf->stm = to_stm_device(dev);

@@ -382,9 +384,10 @@ static int stm_char_open(struct inode *inode, struct file *file)
 	return nonseekable_open(inode, file);

drivers/hwtracing/stm/core.c static int stm_char_open(struct inode *inode, struct file *file) · line 393

Quoted 10 lines

drivers/hwtracing/stm/core.c +2 -1

 
 err_free:
+	kfree(stmf);
+err_put_device:
 	/* matches class_find_device() above */
 	put_device(dev);
-	kfree(stmf);
 
 	return err;
 }

Any comments to this one, Alexander?

Thanks,
Johan

Alexander Shishkin

maintainer

2016-11-18 12:00 UTC #2 lore raw

In reply to Johan Hovold

Quoted 5 lines · Johan Hovold

Make sure to drop the reference taken by class_find_device() also on allocation errors in open().
Fixes 7bd1d4093c2f ("stm class: Introduce an abstraction for...")
Signed-off-by Johan Hovold <[email protected]>

Good catch, thanks! I'm going to change it a bit to look like this if you don't mind:

drivers/hwtracing/stm/core.c +5 -3

index 51f81d64ca..cf6c9ea14c 100644
@@ -361,7 +361,7 @@ static int stm_char_open(struct inode *inode, struct file *file)
        struct stm_file *stmf;
        struct device *dev;
        unsigned int major = imajor(inode);
-       int err = -ENODEV;
+       int err = -ENOMEM;
 
        dev = class_find_device(&stm_class, NULL, &major, major_match);
        if (!dev)

@@ -369,8 +369,9 @@ static int stm_char_open(struct inode *inode, struct file *file)
 
        stmf = kzalloc(sizeof(*stmf), GFP_KERNEL);
        if (!stmf)
-               return -ENOMEM;
+               goto err_put_device;
 
+       err = -ENODEV;
        stm_output_init(&stmf->output);
        stmf->stm = to_stm_device(dev);

@@ -382,9 +383,10 @@ static int stm_char_open(struct inode *inode, struct file *file)
        return nonseekable_open(inode, file);
 
 err_free:
+       kfree(stmf);
+err_put_device:
        /* matches class_find_device() above */
        put_device(dev);
-       kfree(stmf);
 
        return err;
 }

Regards,
--
Alex

Johan Hovold

author

2016-11-18 13:01 UTC #3 lore raw

Quoted 25 lines · Alexander Shishkin

Quoted 5 lines · Johan Hovold
Make sure to drop the reference taken by class_find_device() also on allocation errors in open().
Fixes 7bd1d4093c2f ("stm class: Introduce an abstraction for...")
Signed-off-by Johan Hovold <[email protected]>
Good catch, thanks! I'm going to change it a bit to look like this if you don't mind:
drivers/hwtracing/stm/core.c +1 -1
index 51f81d64ca..cf6c9ea14c 100644
@@ -361,7 +361,7 @@ static int stm_char_open(struct inode *inode, struct file *file)
        struct stm_file *stmf;
        struct device *dev;
        unsigned int major = imajor(inode);
-       int err = -ENODEV;
+       int err = -ENOMEM;
 
        dev = class_find_device(&stm_class, NULL, &major, major_match);
        if (!dev)

drivers/hwtracing/stm/core.c static int stm_char_open(struct inode *inode, struct file *file) · line 376

Quoted 9 lines

drivers/hwtracing/stm/core.c +2 -1

@@ -369,8 +369,9 @@ static int stm_char_open(struct inode *inode, struct file *file)
 
        stmf = kzalloc(sizeof(*stmf), GFP_KERNEL);
        if (!stmf)
-               return -ENOMEM;
+               goto err_put_device;
 
+       err = -ENODEV;
        stm_output_init(&stmf->output);
        stmf->stm = to_stm_device(dev);

Sure. I find it more readable to set the errno in the error path, but I don't mind.

Thanks,
Johan