[PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Kees Cook]

Clang's structure layout randomization feature gets upset when it sees
struct address_space (which is randomized) cast to struct gfs2_glock.
This is due to seeing the mapping pointer as being treated as an array
of gfs2_glock, rather than "something else, before struct address_space":

In file included from fs/gfs2/acl.c:23:
fs/gfs2/meta_io.h:44:12: error: casting from randomized structure pointer type 'struct address_space *' to 'struct gfs2_glock *'
	return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
		^

Replace the instances of open-coded pointer math with container_of()
usage, and update the allocator to match.

Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/lkml/202205041550.naKxwCBj-lkp@intel.com
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Bill Wendling <morbo@google.com>
Cc: cluster-devel@redhat.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
This another fix uncovered by the Clang randstruct series[1], so it'd
probably make more sense to land via my tree. Do you have a preference?
[1] https://lore.kernel.org/all/20220503205503.3054173-1-keescook@chromium.org/
---
 fs/gfs2/glock.h   |  8 +++++++-
 fs/gfs2/main.c    | 10 ++++------
 fs/gfs2/meta_io.h |  2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 4f8642301801..2607c7d26640 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -138,6 +138,12 @@ struct lm_lockops {
 	const match_table_t *lm_tokens;
 };
 
+/* gfs2_glock_get(), "glock" must be first. */
+struct glock_aspace {
+	struct gfs2_glock	glock;
+	struct address_space	mapping;
+};
+
 extern struct workqueue_struct *gfs2_delete_workqueue;
 static inline struct gfs2_holder *gfs2_glock_is_locked_by_me(struct gfs2_glock *gl)
 {
@@ -180,7 +186,7 @@ static inline int gfs2_glock_is_held_shrd(struct gfs2_glock *gl)
 static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
 {
 	if (gl->gl_ops->go_flags & GLOF_ASPACE)
-		return (struct address_space *)(gl + 1);
+		return &(container_of(gl, struct glock_aspace, glock)->mapping);
 	return NULL;
 }
 
diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
index 28d0eb23e18e..984bd60d01db 100644
--- a/fs/gfs2/main.c
+++ b/fs/gfs2/main.c
@@ -62,11 +62,10 @@ static void gfs2_init_glock_once(void *foo)
 
 static void gfs2_init_gl_aspace_once(void *foo)
 {
-	struct gfs2_glock *gl = foo;
-	struct address_space *mapping = (struct address_space *)(gl + 1);
+	struct glock_aspace *gl_aspace = foo;
 
-	gfs2_init_glock_once(gl);
-	address_space_init_once(mapping);
+	gfs2_init_glock_once(&gl_aspace->glock);
+	address_space_init_once(&gl_aspace->mapping);
 }
 
 /**
@@ -104,8 +103,7 @@ static int __init init_gfs2_fs(void)
 		goto fail_cachep1;
 
 	gfs2_glock_aspace_cachep = kmem_cache_create("gfs2_glock(aspace)",
-					sizeof(struct gfs2_glock) +
-					sizeof(struct address_space),
+					sizeof(struct glock_aspace),
 					0, 0, gfs2_init_gl_aspace_once);
 
 	if (!gfs2_glock_aspace_cachep)
diff --git a/fs/gfs2/meta_io.h b/fs/gfs2/meta_io.h
index 21880d72081a..2e2f88cfb7ad 100644
--- a/fs/gfs2/meta_io.h
+++ b/fs/gfs2/meta_io.h
@@ -41,7 +41,7 @@ static inline struct gfs2_sbd *gfs2_mapping2sbd(struct address_space *mapping)
 {
 	struct inode *inode = mapping->host;
 	if (mapping->a_ops == &gfs2_meta_aops)
-		return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
+		return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;
 	else if (mapping->a_ops == &gfs2_rgrp_aops)
 		return container_of(mapping, struct gfs2_sbd, sd_aspace);
 	else
-- 
2.32.0

[Cluster-devel] [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Kees Cook]

Clang's structure layout randomization feature gets upset when it sees
struct address_space (which is randomized) cast to struct gfs2_glock.
This is due to seeing the mapping pointer as being treated as an array
of gfs2_glock, rather than "something else, before struct address_space":

In file included from fs/gfs2/acl.c:23:
fs/gfs2/meta_io.h:44:12: error: casting from randomized structure pointer type 'struct address_space *' to 'struct gfs2_glock *'
	return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
		^

Replace the instances of open-coded pointer math with container_of()
usage, and update the allocator to match.

Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/lkml/202205041550.naKxwCBj-lkp at intel.com
Cc: Bob Peterson <rpeterso@redhat.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Bill Wendling <morbo@google.com>
Cc: cluster-devel at redhat.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
This another fix uncovered by the Clang randstruct series[1], so it'd
probably make more sense to land via my tree. Do you have a preference?
[1] https://lore.kernel.org/all/20220503205503.3054173-1-keescook at chromium.org/
---
 fs/gfs2/glock.h   |  8 +++++++-
 fs/gfs2/main.c    | 10 ++++------
 fs/gfs2/meta_io.h |  2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 4f8642301801..2607c7d26640 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -138,6 +138,12 @@ struct lm_lockops {
 	const match_table_t *lm_tokens;
 };
 
+/* gfs2_glock_get(), "glock" must be first. */
+struct glock_aspace {
+	struct gfs2_glock	glock;
+	struct address_space	mapping;
+};
+
 extern struct workqueue_struct *gfs2_delete_workqueue;
 static inline struct gfs2_holder *gfs2_glock_is_locked_by_me(struct gfs2_glock *gl)
 {
@@ -180,7 +186,7 @@ static inline int gfs2_glock_is_held_shrd(struct gfs2_glock *gl)
 static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
 {
 	if (gl->gl_ops->go_flags & GLOF_ASPACE)
-		return (struct address_space *)(gl + 1);
+		return &(container_of(gl, struct glock_aspace, glock)->mapping);
 	return NULL;
 }
 
diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
index 28d0eb23e18e..984bd60d01db 100644
--- a/fs/gfs2/main.c
+++ b/fs/gfs2/main.c
@@ -62,11 +62,10 @@ static void gfs2_init_glock_once(void *foo)
 
 static void gfs2_init_gl_aspace_once(void *foo)
 {
-	struct gfs2_glock *gl = foo;
-	struct address_space *mapping = (struct address_space *)(gl + 1);
+	struct glock_aspace *gl_aspace = foo;
 
-	gfs2_init_glock_once(gl);
-	address_space_init_once(mapping);
+	gfs2_init_glock_once(&gl_aspace->glock);
+	address_space_init_once(&gl_aspace->mapping);
 }
 
 /**
@@ -104,8 +103,7 @@ static int __init init_gfs2_fs(void)
 		goto fail_cachep1;
 
 	gfs2_glock_aspace_cachep = kmem_cache_create("gfs2_glock(aspace)",
-					sizeof(struct gfs2_glock) +
-					sizeof(struct address_space),
+					sizeof(struct glock_aspace),
 					0, 0, gfs2_init_gl_aspace_once);
 
 	if (!gfs2_glock_aspace_cachep)
diff --git a/fs/gfs2/meta_io.h b/fs/gfs2/meta_io.h
index 21880d72081a..2e2f88cfb7ad 100644
--- a/fs/gfs2/meta_io.h
+++ b/fs/gfs2/meta_io.h
@@ -41,7 +41,7 @@ static inline struct gfs2_sbd *gfs2_mapping2sbd(struct address_space *mapping)
 {
 	struct inode *inode = mapping->host;
 	if (mapping->a_ops == &gfs2_meta_aops)
-		return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
+		return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;
 	else if (mapping->a_ops == &gfs2_rgrp_aops)
 		return container_of(mapping, struct gfs2_sbd, sd_aspace);
 	else
-- 
2.32.0

Re: [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Christoph Hellwig]

> +/* gfs2_glock_get(), "glock" must be first. */
> +struct glock_aspace {
> +	struct gfs2_glock	glock;
> +	struct address_space	mapping;
> +};

Why does glock need to be first?  The whole point of using container_of
is that we don't have to make that assumption.

>  static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
>  {
>  	if (gl->gl_ops->go_flags & GLOF_ASPACE)
> -		return (struct address_space *)(gl + 1);
> +		return &(container_of(gl, struct glock_aspace, glock)->mapping);

Do we even need the braces here?

>  	struct inode *inode = mapping->host;
>  	if (mapping->a_ops == &gfs2_meta_aops)
> -		return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
> +		return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;

A local variable would be really nice for the reader here to decompose
this a bit:

		struct glock_aspace *a =
			container_of(mapping, struct glock_aspace, mapping);

		return a->glock.gl_name.ln_sbd;

[Cluster-devel] [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Christoph Hellwig]

> +/* gfs2_glock_get(), "glock" must be first. */
> +struct glock_aspace {
> +	struct gfs2_glock	glock;
> +	struct address_space	mapping;
> +};

Why does glock need to be first?  The whole point of using container_of
is that we don't have to make that assumption.

>  static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
>  {
>  	if (gl->gl_ops->go_flags & GLOF_ASPACE)
> -		return (struct address_space *)(gl + 1);
> +		return &(container_of(gl, struct glock_aspace, glock)->mapping);

Do we even need the braces here?

>  	struct inode *inode = mapping->host;
>  	if (mapping->a_ops == &gfs2_meta_aops)
> -		return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
> +		return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;

A local variable would be really nice for the reader here to decompose
this a bit:

		struct glock_aspace *a =
			container_of(mapping, struct glock_aspace, mapping);

		return a->glock.gl_name.ln_sbd;

Re: [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Andreas Gruenbacher]

Kees and Christoph,

thanks for the patch and review.

On Tue, May 10, 2022 at 9:29 AM Christoph Hellwig <hch@infradead.org> wrote:
> > +/* gfs2_glock_get(), "glock" must be first. */
> > +struct glock_aspace {
> > +     struct gfs2_glock       glock;
> > +     struct address_space    mapping;
> > +};
>
> Why does glock need to be first?  The whole point of using container_of
> is that we don't have to make that assumption.

Just needs to be cleaned up in gfs2_glock_get() and
gfs2_glock_dealloc() as well. I'll do that when applying the patch.

> >  static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
> >  {
> >       if (gl->gl_ops->go_flags & GLOF_ASPACE)
> > -             return (struct address_space *)(gl + 1);
> > +             return &(container_of(gl, struct glock_aspace, glock)->mapping);
>
> Do we even need the braces here?

Will use a local variable here, as suggested below.

> >       struct inode *inode = mapping->host;
> >       if (mapping->a_ops == &gfs2_meta_aops)
> > -             return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
> > +             return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;
>
> A local variable would be really nice for the reader here to decompose
> this a bit:
>
>                 struct glock_aspace *a =
>                         container_of(mapping, struct glock_aspace, mapping);
>
>                 return a->glock.gl_name.ln_sbd;

Yes.

Thanks,
Andreas

[Cluster-devel] [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Andreas Gruenbacher]

Kees and Christoph,

thanks for the patch and review.

On Tue, May 10, 2022 at 9:29 AM Christoph Hellwig <hch@infradead.org> wrote:
> > +/* gfs2_glock_get(), "glock" must be first. */
> > +struct glock_aspace {
> > +     struct gfs2_glock       glock;
> > +     struct address_space    mapping;
> > +};
>
> Why does glock need to be first?  The whole point of using container_of
> is that we don't have to make that assumption.

Just needs to be cleaned up in gfs2_glock_get() and
gfs2_glock_dealloc() as well. I'll do that when applying the patch.

> >  static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
> >  {
> >       if (gl->gl_ops->go_flags & GLOF_ASPACE)
> > -             return (struct address_space *)(gl + 1);
> > +             return &(container_of(gl, struct glock_aspace, glock)->mapping);
>
> Do we even need the braces here?

Will use a local variable here, as suggested below.

> >       struct inode *inode = mapping->host;
> >       if (mapping->a_ops == &gfs2_meta_aops)
> > -             return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
> > +             return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;
>
> A local variable would be really nice for the reader here to decompose
> this a bit:
>
>                 struct glock_aspace *a =
>                         container_of(mapping, struct glock_aspace, mapping);
>
>                 return a->glock.gl_name.ln_sbd;

Yes.

Thanks,
Andreas

Re: [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Kees Cook]

On Tue, May 10, 2022 at 01:13:31PM +0200, Andreas Gruenbacher wrote:
> Kees and Christoph,
> 
> thanks for the patch and review.
> 
> On Tue, May 10, 2022 at 9:29 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > +/* gfs2_glock_get(), "glock" must be first. */
> > > +struct glock_aspace {
> > > +     struct gfs2_glock       glock;
> > > +     struct address_space    mapping;
> > > +};
> >
> > Why does glock need to be first?  The whole point of using container_of
> > is that we don't have to make that assumption.
> 
> Just needs to be cleaned up in gfs2_glock_get() and
> gfs2_glock_dealloc() as well. I'll do that when applying the patch.
> 
> > >  static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
> > >  {
> > >       if (gl->gl_ops->go_flags & GLOF_ASPACE)
> > > -             return (struct address_space *)(gl + 1);
> > > +             return &(container_of(gl, struct glock_aspace, glock)->mapping);
> >
> > Do we even need the braces here?
> 
> Will use a local variable here, as suggested below.
> 
> > >       struct inode *inode = mapping->host;
> > >       if (mapping->a_ops == &gfs2_meta_aops)
> > > -             return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
> > > +             return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;
> >
> > A local variable would be really nice for the reader here to decompose
> > this a bit:
> >
> >                 struct glock_aspace *a =
> >                         container_of(mapping, struct glock_aspace, mapping);
> >
> >                 return a->glock.gl_name.ln_sbd;
> 
> Yes.

Thanks! So I should leave this with you to arrange, or should I send an
updated patch?

-- 
Kees Cook

[Cluster-devel] [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Kees Cook]

On Tue, May 10, 2022 at 01:13:31PM +0200, Andreas Gruenbacher wrote:
> Kees and Christoph,
> 
> thanks for the patch and review.
> 
> On Tue, May 10, 2022 at 9:29 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > +/* gfs2_glock_get(), "glock" must be first. */
> > > +struct glock_aspace {
> > > +     struct gfs2_glock       glock;
> > > +     struct address_space    mapping;
> > > +};
> >
> > Why does glock need to be first?  The whole point of using container_of
> > is that we don't have to make that assumption.
> 
> Just needs to be cleaned up in gfs2_glock_get() and
> gfs2_glock_dealloc() as well. I'll do that when applying the patch.
> 
> > >  static inline struct address_space *gfs2_glock2aspace(struct gfs2_glock *gl)
> > >  {
> > >       if (gl->gl_ops->go_flags & GLOF_ASPACE)
> > > -             return (struct address_space *)(gl + 1);
> > > +             return &(container_of(gl, struct glock_aspace, glock)->mapping);
> >
> > Do we even need the braces here?
> 
> Will use a local variable here, as suggested below.
> 
> > >       struct inode *inode = mapping->host;
> > >       if (mapping->a_ops == &gfs2_meta_aops)
> > > -             return (((struct gfs2_glock *)mapping) - 1)->gl_name.ln_sbd;
> > > +             return container_of(mapping, struct glock_aspace, mapping)->glock.gl_name.ln_sbd;
> >
> > A local variable would be really nice for the reader here to decompose
> > this a bit:
> >
> >                 struct glock_aspace *a =
> >                         container_of(mapping, struct glock_aspace, mapping);
> >
> >                 return a->glock.gl_name.ln_sbd;
> 
> Yes.

Thanks! So I should leave this with you to arrange, or should I send an
updated patch?

-- 
Kees Cook

Re: [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Andreas Gruenbacher]

Kees,

On Tue, May 10, 2022 at 5:51 PM Kees Cook <keescook@chromium.org> wrote:
> Thanks! So I should leave this with you to arrange, or should I send an
> updated patch?

are you happy with this?

https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/?id=833ab609b94f6

Thanks,
Andreas

[Cluster-devel] [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Andreas Gruenbacher]

Kees,

On Tue, May 10, 2022 at 5:51 PM Kees Cook <keescook@chromium.org> wrote:
> Thanks! So I should leave this with you to arrange, or should I send an
> updated patch?

are you happy with this?

https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/?id=833ab609b94f6

Thanks,
Andreas

Re: [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Kees Cook]

On Tue, May 10, 2022 at 06:16:30PM +0200, Andreas Gruenbacher wrote:
> Kees,
> 
> On Tue, May 10, 2022 at 5:51 PM Kees Cook <keescook@chromium.org> wrote:
> > Thanks! So I should leave this with you to arrange, or should I send an
> > updated patch?
> 
> are you happy with this?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/?id=833ab609b94f6
> 

Sure; looks great! You should credit yourself with the code changes,
maybe, with:

Co-developed-by: Andreas Gruenbacher <agruenba@redhat.com>

Thanks!

-- 
Kees Cook

[Cluster-devel] [PATCH] gfs2: Use container_of() for gfs2_glock(aspace)

[Kees Cook]

On Tue, May 10, 2022 at 06:16:30PM +0200, Andreas Gruenbacher wrote:
> Kees,
> 
> On Tue, May 10, 2022 at 5:51 PM Kees Cook <keescook@chromium.org> wrote:
> > Thanks! So I should leave this with you to arrange, or should I send an
> > updated patch?
> 
> are you happy with this?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/commit/?id=833ab609b94f6
> 

Sure; looks great! You should credit yourself with the code changes,
maybe, with:

Co-developed-by: Andreas Gruenbacher <agruenba@redhat.com>

Thanks!

-- 
Kees Cook