This patch series fixes minor code issues including wrong trace point foramts, meaningless null checking, and possible resource leak in error cases.
This is based drm-next branch.
Seung-Woo Kim (2): drm: fix print format of sequence in trace point drm: move edid null check to the first part of drm_edid_block_valid
YoungJun Cho (1): drm: fix error routines in drm_open_helper
drivers/gpu/drm/drm_edid.c | 5 ++++- drivers/gpu/drm/drm_fops.c | 17 +++++++++++++---- drivers/gpu/drm/drm_trace.h | 6 +++--- 3 files changed, 20 insertions(+), 8 deletions(-)
seq of a trace point is unsigned int but print format was %d. So it fixes the format as %u even the format can be not used.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/drm_trace.h | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_trace.h b/drivers/gpu/drm/drm_trace.h index 03ea964..27cc95f 100644 --- a/drivers/gpu/drm/drm_trace.h +++ b/drivers/gpu/drm/drm_trace.h @@ -21,7 +21,7 @@ TRACE_EVENT(drm_vblank_event, __entry->crtc = crtc; __entry->seq = seq; ), - TP_printk("crtc=%d, seq=%d", __entry->crtc, __entry->seq) + TP_printk("crtc=%d, seq=%u", __entry->crtc, __entry->seq) );
TRACE_EVENT(drm_vblank_event_queued, @@ -37,7 +37,7 @@ TRACE_EVENT(drm_vblank_event_queued, __entry->crtc = crtc; __entry->seq = seq; ), - TP_printk("pid=%d, crtc=%d, seq=%d", __entry->pid, __entry->crtc, \ + TP_printk("pid=%d, crtc=%d, seq=%u", __entry->pid, __entry->crtc, \ __entry->seq) );
@@ -54,7 +54,7 @@ TRACE_EVENT(drm_vblank_event_delivered, __entry->crtc = crtc; __entry->seq = seq; ), - TP_printk("pid=%d, crtc=%d, seq=%d", __entry->pid, __entry->crtc, \ + TP_printk("pid=%d, crtc=%d, seq=%u", __entry->pid, __entry->crtc, \ __entry->seq) );
On Mon, Jul 01, 2013 at 07:06:31PM +0900, Seung-Woo Kim wrote:
seq of a trace point is unsigned int but print format was %d. So it fixes the format as %u even the format can be not used.
I don't understand what you mean here. The patch itself looks fine.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
Hello Chris,
Thank you for reviewing.
On 2013년 07월 01일 19:23, Chris Wilson wrote:
On Mon, Jul 01, 2013 at 07:06:31PM +0900, Seung-Woo Kim wrote:
seq of a trace point is unsigned int but print format was %d. So it fixes the format as %u even the format can be not used.
I don't understand what you mean here. The patch itself looks fine.
I can not find where the format is used or not, so I think the format is not really used anywhere. If you want to fix the commit-msg, I'll update and re-send this patch.
Regards, - Seung-Woo Kim
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Mon, Jul 01, 2013 at 07:28:49PM +0900, Seung-Woo Kim wrote:
Hello Chris,
Thank you for reviewing.
On 2013년 07월 01일 19:23, Chris Wilson wrote:
On Mon, Jul 01, 2013 at 07:06:31PM +0900, Seung-Woo Kim wrote:
seq of a trace point is unsigned int but print format was %d. So it fixes the format as %u even the format can be not used.
I don't understand what you mean here. The patch itself looks fine.
I can not find where the format is used or not, so I think the format is not really used anywhere. If you want to fix the commit-msg, I'll update and re-send this patch.
One of the tricks performed by the TRACE() macro is that it prepends "trace_" to the name of the tracepoint for use in the code.
git grep trace_drm_vblank_event: drivers/gpu/drm/drm_irq.c: trace_drm_vblank_event_delivered(e->base.pid, e->pipe, drivers/gpu/drm/drm_irq.c: trace_drm_vblank_event_queued(current->pid, pipe, drivers/gpu/drm/drm_irq.c: trace_drm_vblank_event(crtc, seq); -Chris
seq of a trace point is unsigned int but print format was %d. So it fixes the format as %u.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- change from v1 - remove wrong commit messageas Chris commented
drivers/gpu/drm/drm_trace.h | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_trace.h b/drivers/gpu/drm/drm_trace.h index 03ea964..27cc95f 100644 --- a/drivers/gpu/drm/drm_trace.h +++ b/drivers/gpu/drm/drm_trace.h @@ -21,7 +21,7 @@ TRACE_EVENT(drm_vblank_event, __entry->crtc = crtc; __entry->seq = seq; ), - TP_printk("crtc=%d, seq=%d", __entry->crtc, __entry->seq) + TP_printk("crtc=%d, seq=%u", __entry->crtc, __entry->seq) );
TRACE_EVENT(drm_vblank_event_queued, @@ -37,7 +37,7 @@ TRACE_EVENT(drm_vblank_event_queued, __entry->crtc = crtc; __entry->seq = seq; ), - TP_printk("pid=%d, crtc=%d, seq=%d", __entry->pid, __entry->crtc, \ + TP_printk("pid=%d, crtc=%d, seq=%u", __entry->pid, __entry->crtc, \ __entry->seq) );
@@ -54,7 +54,7 @@ TRACE_EVENT(drm_vblank_event_delivered, __entry->crtc = crtc; __entry->seq = seq; ), - TP_printk("pid=%d, crtc=%d, seq=%d", __entry->pid, __entry->crtc, \ + TP_printk("pid=%d, crtc=%d, seq=%u", __entry->pid, __entry->crtc, \ __entry->seq) );
On Mon, Jul 01, 2013 at 07:44:14PM +0900, Seung-Woo Kim wrote:
seq of a trace point is unsigned int but print format was %d. So it fixes the format as %u.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
If raw_edid is null, it will crash, so checking in bad label is meaningless.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/drm_edid.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 2dc1a60..dbaed34 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -968,6 +968,9 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) u8 csum = 0; struct edid *edid = (struct edid *)raw_edid;
+ if (!raw_edid) + return 0; + if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6;
@@ -1013,7 +1016,7 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) return 1;
bad: - if (raw_edid && print_bad_edid) { + if (print_bad_edid) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false);
On Mon, Jul 01, 2013 at 07:06:32PM +0900, Seung-Woo Kim wrote:
If raw_edid is null, it will crash, so checking in bad label is meaningless.
It would be an error on part of the caller, but the defense looks sane. As the function is a bool, I would have preferred it returned true/false, but your patch is correct wrt to the rest of the function.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Mon, Jul 1, 2013 at 12:21 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Mon, Jul 01, 2013 at 07:06:32PM +0900, Seung-Woo Kim wrote:
If raw_edid is null, it will crash, so checking in bad label is meaningless.
It would be an error on part of the caller, but the defense looks sane. As the function is a bool, I would have preferred it returned true/false, but your patch is correct wrt to the rest of the function.
If we consider passing a NULL raw_edid here a caller-error, shouldn't this be a WARN on top? And I concur on the s/0/false/ bikeshed, return 0 could be misleading since for errno returning functions that reads as success. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel,
On 2013년 07월 01일 23:56, Daniel Vetter wrote:
On Mon, Jul 1, 2013 at 12:21 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Mon, Jul 01, 2013 at 07:06:32PM +0900, Seung-Woo Kim wrote:
If raw_edid is null, it will crash, so checking in bad label is meaningless.
It would be an error on part of the caller, but the defense looks sane. As the function is a bool, I would have preferred it returned true/false, but your patch is correct wrt to the rest of the function.
If we consider passing a NULL raw_edid here a caller-error, shouldn't this be a WARN on top? And I concur on the s/0/false/ bikeshed, return 0 could be misleading since for errno returning functions that reads as success.
Yes, you are right. WARN_ON() is better because there was no crash until now. and I will also update all return values as false/true instead of 0/1.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
From: YoungJun Cho yj44.cho@samsung.com
There are missing parts to handle error in drm_open_helper(). The priv->minor, assigned by idr_find() which can return NULL, should be checked whether it is NULL or not before referencing it. put_pid(), drm_gem_release(), and drm_prime_destory_file_private() should be called when error happens after their pair functions are called. If an error occurs after executing dev->driver->open() which allocates driver specific per-file private data, then the private data should be released.
Signed-off-by: YoungJun Cho yj44.cho@samsung.com Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- change from v2 - adds put_pid, drm_gem_release, drm_prime_destroy_file_private as Chris's review change from v1 - replaces error value for failure to find the minor as ENODEV as Chris commented
drivers/gpu/drm/drm_fops.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 429e07d..33b1125 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -271,6 +271,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = idr_find(&drm_minors_idr, minor_id); + if (!priv->minor) { + ret = -ENODEV; + goto out_put_pid; + } + priv->ioctl_count = 0; /* for compatibility root is always authenticated */ priv->authenticated = capable(CAP_SYS_ADMIN); @@ -292,7 +297,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, if (dev->driver->open) { ret = dev->driver->open(dev, priv); if (ret < 0) - goto out_free; + goto out_prime_destroy; }
@@ -304,7 +309,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, if (!priv->minor->master) { mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; - goto out_free; + goto out_close; }
priv->is_master = 1; @@ -322,7 +327,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_lock(&dev->struct_mutex); @@ -333,7 +338,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_unlock(&dev->struct_mutex); @@ -367,7 +372,17 @@ static int drm_open_helper(struct inode *inode, struct file *filp, #endif
return 0; - out_free: + +out_close: + if (dev->driver->postclose) + dev->driver->postclose(dev, priv); +out_prime_destroy: + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_destroy_file_private(&priv->prime); + if (dev->driver->driver_feature & DRIVER_GEM) + drm_gem_release(dev, priv); +out_put_pid: + put_pid(priv->pid); kfree(priv); filp->private_data = NULL; return ret;
If raw_edid of drm_edid_block_vaild() is null, it will crash, so checking in bad label is removed and instead assertion is added at the top of the function. The type of return for the function is bool, so it fixes to return true and false instead of 1 and 0.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- chages from v1 - NULL checking is replaced with WARN_ON() as Daniel commented - all return value is replaced as true/false as Chris and Daniel commented
drivers/gpu/drm/drm_edid.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 2dc1a60..1117f02 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -968,6 +968,8 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) u8 csum = 0; struct edid *edid = (struct edid *)raw_edid;
+ WARN_ON(!raw_edid); + if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6;
@@ -1010,15 +1012,15 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1; + return true;
bad: - if (raw_edid && print_bad_edid) { + if (print_bad_edid) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); } - return 0; + return false; } EXPORT_SYMBOL(drm_edid_block_valid);
On Tue, Jul 02, 2013 at 09:52:02AM +0900, Seung-Woo Kim wrote:
If raw_edid of drm_edid_block_vaild() is null, it will crash, so checking in bad label is removed and instead assertion is added at the top of the function. The type of return for the function is bool, so it fixes to return true and false instead of 1 and 0.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
chages from v1
- NULL checking is replaced with WARN_ON() as Daniel commented
- all return value is replaced as true/false as Chris and Daniel commented
drivers/gpu/drm/drm_edid.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 2dc1a60..1117f02 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -968,6 +968,8 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) u8 csum = 0; struct edid *edid = (struct edid *)raw_edid;
- WARN_ON(!raw_edid);
I don't see this buying us anything. All you get is two backtraces instead of one.
if (WARN_ON(!raw_edid)) return false;
would make more sense if we want tolerate programmer errors a bit better. I'm not a huge fan of such defensive programming though since it tends to make it less clear what the function actually expects from you. But here the WARN_ON() would at least make it clear that it's not meant to happen.
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6;
@@ -1010,15 +1012,15 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1;
- return true;
bad:
- if (raw_edid && print_bad_edid) {
- if (print_bad_edid) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); }
- return 0;
- return false;
} EXPORT_SYMBOL(drm_edid_block_valid);
-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hello Ville,
Thanks for comment.
On 2013년 07월 02일 17:29, Ville Syrjälä wrote:
On Tue, Jul 02, 2013 at 09:52:02AM +0900, Seung-Woo Kim wrote:
If raw_edid of drm_edid_block_vaild() is null, it will crash, so checking in bad label is removed and instead assertion is added at the top of the function. The type of return for the function is bool, so it fixes to return true and false instead of 1 and 0.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
chages from v1
- NULL checking is replaced with WARN_ON() as Daniel commented
- all return value is replaced as true/false as Chris and Daniel commented
drivers/gpu/drm/drm_edid.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 2dc1a60..1117f02 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -968,6 +968,8 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) u8 csum = 0; struct edid *edid = (struct edid *)raw_edid;
- WARN_ON(!raw_edid);
I don't see this buying us anything. All you get is two backtraces instead of one.
if (WARN_ON(!raw_edid)) return false;
would make more sense if we want tolerate programmer errors a bit better. I'm not a huge fan of such defensive programming though since it tends to make it less clear what the function actually expects from you. But here the WARN_ON() would at least make it clear that it's not meant to happen.
Ok, it looks better than just WARN_ON(). I'll updated patch as you commented.
Best Regards, - Seung-Woo Kim
if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6;
@@ -1010,15 +1012,15 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1;
- return true;
bad:
- if (raw_edid && print_bad_edid) {
- if (print_bad_edid) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); }
- return 0;
- return false;
} EXPORT_SYMBOL(drm_edid_block_valid);
-- 1.7.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
If raw_edid of drm_edid_block_vaild() is null, it will crash, so checking in bad label is removed and instead assertion is added at the top of the function. The type of return for the function is bool, so it fixes to return true and false instead of 1 and 0.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- change from v2 - check result of WARN_ON() as Ville's comment chages from v1 - NULL checking is replaced with WARN_ON() as Daniel commented - all return value is replaced as true/false as Chris and Daniel commented
drivers/gpu/drm/drm_edid.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 2dc1a60..95d6f4b 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -968,6 +968,9 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) u8 csum = 0; struct edid *edid = (struct edid *)raw_edid;
+ if (WARN_ON(!raw_edid)) + return false; + if (edid_fixup > 8 || edid_fixup < 0) edid_fixup = 6;
@@ -1010,15 +1013,15 @@ bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid) break; }
- return 1; + return true;
bad: - if (raw_edid && print_bad_edid) { + if (print_bad_edid) { printk(KERN_ERR "Raw EDID:\n"); print_hex_dump(KERN_ERR, " \t", DUMP_PREFIX_NONE, 16, 1, raw_edid, EDID_LENGTH, false); } - return 0; + return false; } EXPORT_SYMBOL(drm_edid_block_valid);
On Tue, Jul 02, 2013 at 05:57:04PM +0900, Seung-Woo Kim wrote:
If raw_edid of drm_edid_block_vaild() is null, it will crash, so checking in bad label is removed and instead assertion is added at the top of the function. The type of return for the function is bool, so it fixes to return true and false instead of 1 and 0.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Tue, Jul 02, 2013 at 09:52:02AM +0900, Seung-Woo Kim wrote:
If raw_edid of drm_edid_block_vaild() is null, it will crash, so checking in bad label is removed and instead assertion is added at the top of the function. The type of return for the function is bool, so it fixes to return true and false instead of 1 and 0.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
if (WARN_ON(raw_edid == NULL)) return false;
Otherwise it is just a WARN_ON() followed by a BUG() :) -Chris
From: YoungJun Cho yj44.cho@samsung.com
There are wrong cases to handle error in drm_open_helper(). The priv->minor, assigned by idr_find() which can return NULL, should be checked whether it is NULL or not before referencing it. And if an error occurs after executing dev->driver->open() which allocates driver specific per-file private data, then the private data should be released.
Signed-off-by: YoungJun Cho yj44.cho@samsung.com Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/drm_fops.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 429e07d..0470261 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -271,6 +271,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = idr_find(&drm_minors_idr, minor_id); + if (!priv->minor) { + ret = -ENOMEM; + goto out_free; + } + priv->ioctl_count = 0; /* for compatibility root is always authenticated */ priv->authenticated = capable(CAP_SYS_ADMIN); @@ -304,7 +309,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, if (!priv->minor->master) { mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; - goto out_free; + goto out_close; }
priv->is_master = 1; @@ -322,7 +327,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_lock(&dev->struct_mutex); @@ -333,7 +338,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_unlock(&dev->struct_mutex); @@ -367,7 +372,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, #endif
return 0; - out_free: + +out_close: + if (dev->driver->postclose) + dev->driver->postclose(dev, priv); +out_free: kfree(priv); filp->private_data = NULL; return ret;
On Mon, Jul 01, 2013 at 07:06:33PM +0900, Seung-Woo Kim wrote:
From: YoungJun Cho yj44.cho@samsung.com
There are wrong cases to handle error in drm_open_helper(). The priv->minor, assigned by idr_find() which can return NULL, should be checked whether it is NULL or not before referencing it. And if an error occurs after executing dev->driver->open() which allocates driver specific per-file private data, then the private data should be released.
Signed-off-by: YoungJun Cho yj44.cho@samsung.com Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/drm_fops.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 429e07d..0470261 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -271,6 +271,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = idr_find(&drm_minors_idr, minor_id);
- if (!priv->minor) {
ret = -ENOMEM;
Elsewhere we use ENODEV for a failure to find the minor inode.
The error path cleanup changes look reasonable. Though require a quick audit to make sure all of the callees do not expect more state to be correctly setup before being called. -Chris
From: YoungJun Cho yj44.cho@samsung.com
There are wrong cases to handle error in drm_open_helper(). The priv->minor, assigned by idr_find() which can return NULL, should be checked whether it is NULL or not before referencing it. And if an error occurs after executing dev->driver->open() which allocates driver specific per-file private data, then the private data should be released.
Signed-off-by: YoungJun Cho yj44.cho@samsung.com Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- change from v1 - replace error value for failure to find the minor as ENODEV as Chris commented
drivers/gpu/drm/drm_fops.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 429e07d..0470261 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -271,6 +271,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = idr_find(&drm_minors_idr, minor_id); + if (!priv->minor) { + ret = -ENODEV; + goto out_free; + } + priv->ioctl_count = 0; /* for compatibility root is always authenticated */ priv->authenticated = capable(CAP_SYS_ADMIN); @@ -304,7 +309,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, if (!priv->minor->master) { mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; - goto out_free; + goto out_close; }
priv->is_master = 1; @@ -322,7 +327,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_lock(&dev->struct_mutex); @@ -333,7 +338,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_unlock(&dev->struct_mutex); @@ -367,7 +372,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, #endif
return 0; - out_free: + +out_close: + if (dev->driver->postclose) + dev->driver->postclose(dev, priv); +out_free: kfree(priv); filp->private_data = NULL; return ret;
On Mon, Jul 01, 2013 at 07:49:10PM +0900, Seung-Woo Kim wrote:
+out_close:
- if (dev->driver->postclose)
dev->driver->postclose(dev, priv);+out_free: kfree(priv); filp->private_data = NULL; return ret;
Looks like we are also missing:
if (drm_core_check_feature(dev, DRIVER_PRIME)) drm_prime_destroy_file_private(&file_priv->prime);
put_pid(file_priv->pid);
after out_free. -Chris
Hello Chris,
On 2013년 07월 01일 19:57, Chris Wilson wrote:
On Mon, Jul 01, 2013 at 07:49:10PM +0900, Seung-Woo Kim wrote:
+out_close:
- if (dev->driver->postclose)
dev->driver->postclose(dev, priv);+out_free: kfree(priv); filp->private_data = NULL; return ret;
Looks like we are also missing:
if (drm_core_check_feature(dev, DRIVER_PRIME)) drm_prime_destroy_file_private(&file_priv->prime);
Currently, file_priv->prime is just initialized, and drm_prime_destroy_file_private() just checks the list is empty and at the open time, prime list is always empty. So IMHO, it seems unnecessary to call drm_prime_destroy_file_private().
If this is necessary, drm_gem_release() is also needed because the pair function of drm_gem_open() is drm_gem_release().
put_pid(file_priv->pid);
Yes, you are rignt. put_pid is also needed.
After discussion about above part, I'll post v3 for this.
Thanks and Regards, - Seung-Woo Kim
after out_free. -Chris
On Mon, Jul 01, 2013 at 08:14:42PM +0900, Seung-Woo Kim wrote:
Hello Chris,
On 2013년 07월 01일 19:57, Chris Wilson wrote:
On Mon, Jul 01, 2013 at 07:49:10PM +0900, Seung-Woo Kim wrote:
+out_close:
- if (dev->driver->postclose)
dev->driver->postclose(dev, priv);+out_free: kfree(priv); filp->private_data = NULL; return ret;
Looks like we are also missing:
if (drm_core_check_feature(dev, DRIVER_PRIME)) drm_prime_destroy_file_private(&file_priv->prime);
Currently, file_priv->prime is just initialized, and drm_prime_destroy_file_private() just checks the list is empty and at the open time, prime list is always empty. So IMHO, it seems unnecessary to call drm_prime_destroy_file_private().
If this is necessary, drm_gem_release() is also needed because the pair function of drm_gem_open() is drm_gem_release().
Hmm, could be a slight ordering bug in drm_release(), but yes I agree that we should also call drm_gem_release() here in drm_open_helper. It is better for the code to be symmetric in cleaning up anything that we create so that we are correct for future changes. We might as well fix it correctly, rather than having to rediscover this bug at some later date. -Chris
Hello Chris,
On Jul 1, 2013 8:53 PM, "Chris Wilson" chris@chris-wilson.co.uk wrote:
On Mon, Jul 01, 2013 at 08:14:42PM +0900, Seung-Woo Kim wrote:
Hello Chris,
On 2013년 07월 01일 19:57, Chris Wilson wrote:
On Mon, Jul 01, 2013 at 07:49:10PM +0900, Seung-Woo Kim wrote:
+out_close:
- if (dev->driver->postclose)
dev->driver->postclose(dev, priv);+out_free: kfree(priv); filp->private_data = NULL; return ret;
Looks like we are also missing:
if (drm_core_check_feature(dev, DRIVER_PRIME)) drm_prime_destroy_file_private(&file_priv->prime);
Currently, file_priv->prime is just initialized, and drm_prime_destroy_file_private() just checks the list is empty and at the open time, prime list is always empty. So IMHO, it seems unnecessary to call drm_prime_destroy_file_private().
If this is necessary, drm_gem_release() is also needed because the pair function of drm_gem_open() is drm_gem_release().
Hmm, could be a slight ordering bug in drm_release(), but yes I agree that we should also call drm_gem_release() here in drm_open_helper. It is better for the code to be symmetric in cleaning up anything that we create so that we are correct for future changes. We might as well fix it correctly, rather than having to rediscover this bug at some later date. -Chris
Thank you for quick reviews. We'll post V3 patch with this also.
Best regards YJ
-- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
From: YoungJun Cho yj44.cho@samsung.com
There are missing parts to handle error in drm_open_helper(). The priv->minor, assigned by idr_find() which can return NULL, should be checked whether it is NULL or not before referencing it. put_pid(), drm_gem_release(), and drm_prime_destory_file_private() should be called when error happens after their pair functions are called. If an error occurs after executing dev->driver->open() which allocates driver specific per-file private data, then the private data should be released.
Signed-off-by: YoungJun Cho yj44.cho@samsung.com Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- change from v3 - fix typo change from v2 - adds put_pid, drm_gem_release, drm_prime_destroy_file_private as Chris's review change from v1 - replaces error value for failure to find the minor as ENODEV as Chris commented
drivers/gpu/drm/drm_fops.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 429e07d..33b1125 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -271,6 +271,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = idr_find(&drm_minors_idr, minor_id); + if (!priv->minor) { + ret = -ENODEV; + goto out_put_pid; + } + priv->ioctl_count = 0; /* for compatibility root is always authenticated */ priv->authenticated = capable(CAP_SYS_ADMIN); @@ -292,7 +297,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, if (dev->driver->open) { ret = dev->driver->open(dev, priv); if (ret < 0) - goto out_free; + goto out_prime_destroy; }
@@ -304,7 +309,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, if (!priv->minor->master) { mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; - goto out_free; + goto out_close; }
priv->is_master = 1; @@ -322,7 +327,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_lock(&dev->struct_mutex); @@ -333,7 +338,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_unlock(&dev->struct_mutex); @@ -367,7 +372,17 @@ static int drm_open_helper(struct inode *inode, struct file *filp, #endif
return 0; - out_free: + +out_close: + if (dev->driver->postclose) + dev->driver->postclose(dev, priv); +out_prime_destroy: + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_destroy_file_private(&priv->prime); + if (dev->driver->driver_features & DRIVER_GEM) + drm_gem_release(dev, priv); +out_put_pid: + put_pid(priv->pid); kfree(priv); filp->private_data = NULL; return ret;
On Tue, Jul 02, 2013 at 09:53:28AM +0900, Seung-Woo Kim wrote:
From: YoungJun Cho yj44.cho@samsung.com
There are missing parts to handle error in drm_open_helper(). The priv->minor, assigned by idr_find() which can return NULL, should be checked whether it is NULL or not before referencing it. put_pid(), drm_gem_release(), and drm_prime_destory_file_private() should be called when error happens after their pair functions are called. If an error occurs after executing dev->driver->open() which allocates driver specific per-file private data, then the private data should be released.
Signed-off-by: YoungJun Cho yj44.cho@samsung.com Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
I can't see anything else that we setup and miss freeing along the failed open, so if it compiles ;-) Reviewed-by: Chris Wilson <chris-wilson.co.uk> -Chris
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Seung-Woo Kim -
Ville Syrjälä -
YoungJun Cho