Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1604296 > unrolled thread

[PATCH 1/3] drm/i915: allocate mock file pointer dynamically

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] drm/i915: allocate mock file pointer dynamically Arnd Bergmann <arnd@arndb.de> - 2017-03-20 10:50 +0100
    Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically Daniel Vetter <daniel@ffwll.ch> - 2017-03-20 11:00 +0100
    Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically Joonas Lahtinen <joonas.lahtinen@linux.intel.com> - 2017-03-20 13:10 +0100
      Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically Arnd Bergmann <arnd@arndb.de> - 2017-03-20 13:10 +0100
      Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically Arnd Bergmann <arnd@arndb.de> - 2017-03-20 13:40 +0100
    Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically Chris Wilson <chris@chris-wilson.co.uk> - 2017-03-21 11:20 +0100

#1604296 — [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

A struct file is a bit too large to put on the kernel stack in general
and triggers a warning for low settings of CONFIG_FRAME_WARN:

drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file':
drivers/gpu/drm/i915/selftests/mock_drm.c:46:1: error: the frame size of 1328 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file_free':
drivers/gpu/drm/i915/selftests/mock_drm.c:54:1: error: the frame size of 1312 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]

It's also slightly dangerous to leave a reference to a stack object
in the drm_file structure after leaving the stack frame.
This changes the code to just allocate the object dynamically
and release it when we are done with it.

Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
index 113dec05c7dc..18514065c93d 100644
--- a/drivers/gpu/drm/i915/selftests/mock_drm.c
+++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
@@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
 struct drm_file *mock_file(struct drm_i915_private *i915)
 {
 	struct inode inode = fake_inode(i915);
-	struct file filp = {};
+	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
 	struct drm_file *file;
 	int err;
 
-	err = drm_open(&inode, &filp);
+	err = drm_open(&inode, filp);
 	if (unlikely(err))
 		return ERR_PTR(err);
 
-	file = filp.private_data;
+	file = filp->private_data;
 	file->authenticated = true;
 	return file;
 }
@@ -48,7 +48,9 @@ struct drm_file *mock_file(struct drm_i915_private *i915)
 void mock_file_free(struct drm_i915_private *i915, struct drm_file *file)
 {
 	struct inode inode = fake_inode(i915);
-	struct file filp = { .private_data = file };
+	struct file *filp = file->filp;
 
-	drm_release(&inode, &filp);
+	drm_release(&inode, filp);
+
+	kfree(filp);
 }
-- 
2.9.0

[toc] | [next] | [standalone]

#1604308

On Mon, Mar 20, 2017 at 10:40:27AM +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file':
> drivers/gpu/drm/i915/selftests/mock_drm.c:46:1: error: the frame size of 1328 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file_free':
> drivers/gpu/drm/i915/selftests/mock_drm.c:54:1: error: the frame size of 1312 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = {};
> +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>  	struct drm_file *file;
>  	int err;
>  
> -	err = drm_open(&inode, &filp);
> +	err = drm_open(&inode, filp);

Aside: We should have a FIXME and/or merge David Herrmanns code to allow a
drm_file without a real struct file for kernel-internal use ...
-Daniel

>  	if (unlikely(err))
>  		return ERR_PTR(err);
>  
> -	file = filp.private_data;
> +	file = filp->private_data;
>  	file->authenticated = true;
>  	return file;
>  }
> @@ -48,7 +48,9 @@ struct drm_file *mock_file(struct drm_i915_private *i915)
>  void mock_file_free(struct drm_i915_private *i915, struct drm_file *file)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = { .private_data = file };
> +	struct file *filp = file->filp;
>  
> -	drm_release(&inode, &filp);
> +	drm_release(&inode, filp);
> +
> +	kfree(filp);
>  }
> -- 
> 2.9.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1604446

On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file':
> drivers/gpu/drm/i915/selftests/mock_drm.c:46:1: error: the frame size of 1328 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file_free':
> drivers/gpu/drm/i915/selftests/mock_drm.c:54:1: error: the frame size of 1312 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

<SNIP>

> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
> >  	struct inode inode = fake_inode(i915);
> > -	struct file filp = {};
> > +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
> >  	struct drm_file *file;
> >  	int err;
>  

	filp = kzalloc(sizeof(*filp), GFP_KERNEL);
	if (unlikely(!filp)) {
		err = -ENOMEM;
		goto err;
	}

And appropriate onion teardown in case drm_open fails, so that we don't
leak memory.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1604447

On Mon, Mar 20, 2017 at 1:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Mar 20, 2017 at 1:00 PM, Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com> wrote:
>> On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
>
>>> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> index 113dec05c7dc..18514065c93d 100644
>>> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>>>  struct drm_file *mock_file(struct drm_i915_private *i915)
>>>  {
>>> >     struct inode inode = fake_inode(i915);
>>> > -   struct file filp = {};
>>> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>>> >     struct drm_file *file;
>>> >     int err;
>>>
>>
>>         filp = kzalloc(sizeof(*filp), GFP_KERNEL);
>>         if (unlikely(!filp)) {
>>                 err = -ENOMEM;
>>                 goto err;
>>         }
>>
>> And appropriate onion teardown in case drm_open fails, so that we don't
>> leak memory.
>
> Oops, of course you are right, sorry about that.

Actually it's even worse, as I had originally planned to also allocate the inode
dynamically, but for some reasons didn't do that in the patch I sent.

This version of the patch as I sent it is rather bogus, so please ignore it
(but not the other two patches I sent). If David's solution to this problem
can make it into 4.12, there is no problem at all. Another alternative
would be to use anon_inode_getfile(), again with proper error handling
and cleanup. This is a bit slower but gives us valid file and inode structures.

     Arnd

[toc] | [prev] | [next] | [standalone]

#1604477

On Mon, Mar 20, 2017 at 1:00 PM, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:

>> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
>> index 113dec05c7dc..18514065c93d 100644
>> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
>> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
>> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>>  struct drm_file *mock_file(struct drm_i915_private *i915)
>>  {
>> >     struct inode inode = fake_inode(i915);
>> > -   struct file filp = {};
>> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>> >     struct drm_file *file;
>> >     int err;
>>
>
>         filp = kzalloc(sizeof(*filp), GFP_KERNEL);
>         if (unlikely(!filp)) {
>                 err = -ENOMEM;
>                 goto err;
>         }
>
> And appropriate onion teardown in case drm_open fails, so that we don't
> leak memory.

Oops, of course you are right, sorry about that.

    Arnd

[toc] | [prev] | [next] | [standalone]

#1605482

On Mon, Mar 20, 2017 at 10:40:27AM +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file':
> drivers/gpu/drm/i915/selftests/mock_drm.c:46:1: error: the frame size of 1328 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file_free':
> drivers/gpu/drm/i915/selftests/mock_drm.c:54:1: error: the frame size of 1312 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = {};
> +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>  	struct drm_file *file;
>  	int err;
>  
> -	err = drm_open(&inode, &filp);
> +	err = drm_open(&inode, filp);
>  	if (unlikely(err))
>  		return ERR_PTR(err);
>  
> -	file = filp.private_data;
> +	file = filp->private_data;

What I don't like about this approach is that we leak the unwanted
inode/file. We only want the drm_file here and any access to above that
inside i915.ko is fubar.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web