[PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Boris Brezillon]

I've spent hours chasing a memory corruption that was caused by
insertion of an extra field field before ->base. Let's document the
fact that base has to be the first field in panfrost_gem_object.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v2:
* Use the proper prefix in the subject line
---
 drivers/gpu/drm/panfrost/panfrost_gem.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index b3517ff9630c..d480261fc177 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -10,6 +10,10 @@
 struct panfrost_mmu;
 
 struct panfrost_gem_object {
+	/*
+	 * Must be the first element because we're using some of the
+	 * drm_gem_shmem helpers.
+	 */
 	struct drm_gem_shmem_object base;
 	struct sg_table *sgts;
 
-- 
2.23.0

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

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Steven Price]

On 29/11/2019 13:56, Boris Brezillon wrote:
> I've spent hours chasing a memory corruption that was caused by
> insertion of an extra field field before ->base. Let's document the
> fact that base has to be the first field in panfrost_gem_object.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

This seems to be a limitation imposed by the gem_create_object()
callback - e.g. it's assumed that kfree() can be directly called on the
result. Useful to have the documentation though.

Reviewed-by: Steven Price <steven.price@arm.com>

Steve

> ---
> Changes in v2:
> * Use the proper prefix in the subject line
> ---
>  drivers/gpu/drm/panfrost/panfrost_gem.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index b3517ff9630c..d480261fc177 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -10,6 +10,10 @@
>  struct panfrost_mmu;
>  
>  struct panfrost_gem_object {
> +	/*
> +	 * Must be the first element because we're using some of the
> +	 * drm_gem_shmem helpers.
> +	 */
>  	struct drm_gem_shmem_object base;
>  	struct sg_table *sgts;
>  
> 

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

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Boris Brezillon]

On Fri, 29 Nov 2019 14:13:33 +0000
Steven Price <steven.price@arm.com> wrote:

> On 29/11/2019 13:56, Boris Brezillon wrote:
> > I've spent hours chasing a memory corruption that was caused by
> > insertion of an extra field field before ->base. Let's document the
> > fact that base has to be the first field in panfrost_gem_object.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> This seems to be a limitation imposed by the gem_create_object()
> callback - e.g. it's assumed that kfree() can be directly called on the
> result. Useful to have the documentation though.

Oh, you're right, I didn't catch that one.

> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> Steve
> 
> > ---
> > Changes in v2:
> > * Use the proper prefix in the subject line
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_gem.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > index b3517ff9630c..d480261fc177 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > @@ -10,6 +10,10 @@
> >  struct panfrost_mmu;
> >  
> >  struct panfrost_gem_object {
> > +	/*
> > +	 * Must be the first element because we're using some of the
> > +	 * drm_gem_shmem helpers.
> > +	 */
> >  	struct drm_gem_shmem_object base;
> >  	struct sg_table *sgts;
> >  
> >   
> 

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

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Daniel Vetter]

On Fri, Nov 29, 2019 at 03:19:36PM +0100, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 14:13:33 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
> > On 29/11/2019 13:56, Boris Brezillon wrote:
> > > I've spent hours chasing a memory corruption that was caused by
> > > insertion of an extra field field before ->base. Let's document the
> > > fact that base has to be the first field in panfrost_gem_object.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> > 
> > This seems to be a limitation imposed by the gem_create_object()
> > callback - e.g. it's assumed that kfree() can be directly called on the
> > result. Useful to have the documentation though.
> 
> Oh, you're right, I didn't catch that one.

As a general rule of thumb, never insert anything before a struct member
called base. Even more so if it's of the same kind of $thing, but less
spezialed. This pattern is so common it's fairly often not documented
anywhere.
-Daniel

> 
> > 
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > 
> > Steve
> > 
> > > ---
> > > Changes in v2:
> > > * Use the proper prefix in the subject line
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_gem.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > > index b3517ff9630c..d480261fc177 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > > @@ -10,6 +10,10 @@
> > >  struct panfrost_mmu;
> > >  
> > >  struct panfrost_gem_object {
> > > +	/*
> > > +	 * Must be the first element because we're using some of the
> > > +	 * drm_gem_shmem helpers.
> > > +	 */
> > >  	struct drm_gem_shmem_object base;
> > >  	struct sg_table *sgts;
> > >  
> > >   
> > 
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Boris Brezillon]

On Fri, 29 Nov 2019 19:40:38 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Nov 29, 2019 at 03:19:36PM +0100, Boris Brezillon wrote:
> > On Fri, 29 Nov 2019 14:13:33 +0000
> > Steven Price <steven.price@arm.com> wrote:
> >   
> > > On 29/11/2019 13:56, Boris Brezillon wrote:  
> > > > I've spent hours chasing a memory corruption that was caused by
> > > > insertion of an extra field field before ->base. Let's document the
> > > > fact that base has to be the first field in panfrost_gem_object.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
> > > 
> > > This seems to be a limitation imposed by the gem_create_object()
> > > callback - e.g. it's assumed that kfree() can be directly called on the
> > > result. Useful to have the documentation though.  
> > 
> > Oh, you're right, I didn't catch that one.  
> 
> As a general rule of thumb, never insert anything before a struct member
> called base. Even more so if it's of the same kind of $thing, but less
> spezialed. This pattern is so common it's fairly often not documented
> anywhere.

I could argue that anything using container_of() in its to_xxx() helper
is misleading the user into thinking the position of the base field
doesn't matter (which is exactly what happened here), but I feel like I
already lost this battle, so I'll simply drop the patch.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Ville Syrjälä]

On Fri, Nov 29, 2019 at 08:24:37PM +0100, Boris Brezillon wrote:
> On Fri, 29 Nov 2019 19:40:38 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Nov 29, 2019 at 03:19:36PM +0100, Boris Brezillon wrote:
> > > On Fri, 29 Nov 2019 14:13:33 +0000
> > > Steven Price <steven.price@arm.com> wrote:
> > >   
> > > > On 29/11/2019 13:56, Boris Brezillon wrote:  
> > > > > I've spent hours chasing a memory corruption that was caused by
> > > > > insertion of an extra field field before ->base. Let's document the
> > > > > fact that base has to be the first field in panfrost_gem_object.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>    
> > > > 
> > > > This seems to be a limitation imposed by the gem_create_object()
> > > > callback - e.g. it's assumed that kfree() can be directly called on the
> > > > result. Useful to have the documentation though.  
> > > 
> > > Oh, you're right, I didn't catch that one.  
> > 
> > As a general rule of thumb, never insert anything before a struct member
> > called base. Even more so if it's of the same kind of $thing, but less
> > spezialed. This pattern is so common it's fairly often not documented
> > anywhere.
> 
> I could argue that anything using container_of() in its to_xxx() helper
> is misleading the user into thinking the position of the base field
> doesn't matter (which is exactly what happened here), but I feel like I
> already lost this battle, so I'll simply drop the patch.

I agree that this is a bit annoying. Apart from the "let's pass
the wrong thing to kfree()" issue the other problem is NULL
pointers. If those don't get preserved when going between the
base and derived class the code will turn to bad spaghetti.

Not a problem for the typical to_foo_crtc() since we could
hide an explicit NULL check in there. But the other direction
generally just uses &foo_crtc->base so not going to work.
Shouldn't be an impossible task to wrap all of those as well,
but I guess no one's motivated enough to actually do it.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Boris Brezillon]

On Fri, 29 Nov 2019 21:43:45 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Nov 29, 2019 at 08:24:37PM +0100, Boris Brezillon wrote:
> > On Fri, 29 Nov 2019 19:40:38 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > On Fri, Nov 29, 2019 at 03:19:36PM +0100, Boris Brezillon wrote:  
> > > > On Fri, 29 Nov 2019 14:13:33 +0000
> > > > Steven Price <steven.price@arm.com> wrote:
> > > >     
> > > > > On 29/11/2019 13:56, Boris Brezillon wrote:    
> > > > > > I've spent hours chasing a memory corruption that was caused by
> > > > > > insertion of an extra field field before ->base. Let's document the
> > > > > > fact that base has to be the first field in panfrost_gem_object.
> > > > > > 
> > > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>      
> > > > > 
> > > > > This seems to be a limitation imposed by the gem_create_object()
> > > > > callback - e.g. it's assumed that kfree() can be directly called on the
> > > > > result. Useful to have the documentation though.    
> > > > 
> > > > Oh, you're right, I didn't catch that one.    
> > > 
> > > As a general rule of thumb, never insert anything before a struct member
> > > called base. Even more so if it's of the same kind of $thing, but less
> > > spezialed. This pattern is so common it's fairly often not documented
> > > anywhere.  
> > 
> > I could argue that anything using container_of() in its to_xxx() helper
> > is misleading the user into thinking the position of the base field
> > doesn't matter (which is exactly what happened here), but I feel like I
> > already lost this battle, so I'll simply drop the patch.  
> 
> I agree that this is a bit annoying. Apart from the "let's pass
> the wrong thing to kfree()"

This is already a disputable design choice IMHO. Why should you
delegate the free() to someone who didn't allocate the thing in the
first place.

> issue the other problem is NULL
> pointers. If those don't get preserved when going between the
> base and derived class the code will turn to bad spaghetti.
> 
> Not a problem for the typical to_foo_crtc() since we could
> hide an explicit NULL check in there. But the other direction
> generally just uses &foo_crtc->base so not going to work.

Well, if you inherit from the base struct, you'll inevitably have
NULL checks because of pointer dereferences you have when accessing
other fields, so I'd expects most of them to be already present (not
saying finding the remaining ones is an easy task).

The real problem I see here is that we re-use helpers manipulating base
objects without providing wrappers, and that can only work if base is
the first field in the child struct.

> Shouldn't be an impossible task to wrap all of those as well,
> but I guess no one's motivated enough to actually do it.
> 

Actually, I never suggested to do any of that, because I know how
invasive/risky such a change would be. All I was trying to do was make
an implicit requirement more explicit with a comment stating the fact
and explaining why (BTW, I like your suggestion to use BUILD_BUG_ON()
to catch those problems at compilation time).

Anyway, enough on that matter. I'll try to remember that the use of
container_of() does not necessarily means fields can be shuffled.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Alyssa Rosenzweig]

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

Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

On Fri, Nov 29, 2019 at 02:56:14PM +0100, Boris Brezillon wrote:
> I've spent hours chasing a memory corruption that was caused by
> insertion of an extra field field before ->base. Let's document the
> fact that base has to be the first field in panfrost_gem_object.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v2:
> * Use the proper prefix in the subject line
> ---
>  drivers/gpu/drm/panfrost/panfrost_gem.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index b3517ff9630c..d480261fc177 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -10,6 +10,10 @@
>  struct panfrost_mmu;
>  
>  struct panfrost_gem_object {
> +	/*
> +	 * Must be the first element because we're using some of the
> +	 * drm_gem_shmem helpers.
> +	 */
>  	struct drm_gem_shmem_object base;
>  	struct sg_table *sgts;
>  
> -- 
> 2.23.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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

Re: [PATCH v2] drm/panfrost: Document base field location constraint in panfrost_gem_object

[Ville Syrjälä]

On Fri, Nov 29, 2019 at 02:56:14PM +0100, Boris Brezillon wrote:
> I've spent hours chasing a memory corruption that was caused by
> insertion of an extra field field before ->base. Let's document the
> fact that base has to be the first field in panfrost_gem_object.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v2:
> * Use the proper prefix in the subject line
> ---
>  drivers/gpu/drm/panfrost/panfrost_gem.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index b3517ff9630c..d480261fc177 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -10,6 +10,10 @@
>  struct panfrost_mmu;
>  
>  struct panfrost_gem_object {
> +	/*
> +	 * Must be the first element because we're using some of the
> +	 * drm_gem_shmem helpers.
> +	 */

An accompanying BUILD_BUG_ON() would be a good way to catch anyone
ignoring the comment.

>  	struct drm_gem_shmem_object base;
>  	struct sg_table *sgts;
>  
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel