[PATCH 0/3] drm: minor cleanups

[PATCH 0/3] drm: minor cleanups

[Seung-Woo Kim]

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(-)

-- 
1.7.4.1

[PATCH 1/3] drm: fix print format of sequence in trace point

[Seung-Woo Kim]

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)
 );
 
-- 
1.7.4.1

[PATCH 2/3] drm: move edid null check to the first part of drm_edid_block_valid

[Seung-Woo Kim]

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);
-- 
1.7.4.1

[PATCH 3/3] drm: fix error routines in drm_open_helper

[Seung-Woo Kim]

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;
-- 
1.7.4.1

Re: [PATCH 3/3] drm: fix error routines in drm_open_helper

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm: move edid null check to the first part of drm_edid_block_valid

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm: fix print format of sequence in trace point

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm: fix print format of sequence in trace point

[Seung-Woo Kim]

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
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: fix print format of sequence in trace point

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/3] drm: fix print format of sequence in trace point

[Seung-Woo Kim]

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)
 );
 
-- 
1.7.4.1

Re: [PATCH v2 1/3] drm: fix print format of sequence in trace point

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2 3/3] drm: fix error routines in drm_open_helper

[Seung-Woo Kim]

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;
-- 
1.7.4.1

Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper

[Seung-Woo Kim]

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
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper

[YoungJun Cho]

[-- Attachment #1.1: Type: text/plain, Size: 1809 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 2604 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: move edid null check to the first part of drm_edid_block_valid

[Daniel Vetter]

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

Re: [PATCH 2/3] drm: move edid null check to the first part of drm_edid_block_valid

[Seung-Woo Kim]

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
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/3] drm: fix error routines in drm_open_helper

[Seung-Woo Kim]

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;
-- 
1.7.9.5

[PATCH v2 2/3] drm: add assertion for checking null edid to drm_edid_block_valid

[Seung-Woo Kim]

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);
 
-- 
1.7.4.1

[PATCH v4 3/3] drm: fix error routines in drm_open_helper

[Seung-Woo Kim]

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;
-- 
1.7.9.5

Re: [PATCH v2 2/3] drm: add assertion for checking null edid to drm_edid_block_valid

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 2/3] drm: add assertion for checking null edid to drm_edid_block_valid

[Seung-Woo Kim]

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
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/3] drm: add assertion for checking null edid to drm_edid_block_valid

[Seung-Woo Kim]

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);
 
-- 
1.7.4.1

Re: [PATCH v4 3/3] drm: fix error routines in drm_open_helper

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 2/3] drm: add assertion for checking null edid to drm_edid_block_valid

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 2/3] drm: add assertion for checking null edid to drm_edid_block_valid

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre