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

[YoungJun Cho]

Hello Chris,

On Jul 1, 2013 8:53 PM, "Chris Wilson"  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(_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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- next part --
An HTML attachment was scrubbed...
URL: 

[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(_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 Center
--

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

[Seung-Woo Kim]

From: YoungJun Cho 

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 
Signed-off-by: Seung-Woo Kim 
Signed-off-by: Kyungmin Park 
---
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(_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(>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(>minor->master);
drm_master_put(>master);
mutex_unlock(>struct_mutex);
-   goto out_free;
+   goto out_close;
}
}
mutex_lock(>struct_mutex);
@@ -333,7 +338,7 @@ static int drm_open_helper(struct inode *inode, struct file 
*filp,
drm_master_put(>minor->master);
drm_master_put(>master);
mutex_unlock(>struct_mutex);
-   goto out_free;
+   goto out_close;
}
}
mutex_unlock(>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

[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(_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

[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(_priv->prime);

put_pid(file_priv->pid);

after out_free.
-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@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@samsung.com
Signed-off-by: Seung-Woo Kim sw0312@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@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

___
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 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
___
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

[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 RD 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]

Hello Chris,

On Jul 1, 2013 8:53 PM, Chris Wilson ch...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel