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 <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]
Cc: Bob Peterson <[email protected]>
Cc: Andreas Gruenbacher <[email protected]>
Cc: Bill Wendling <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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/[email protected]/
---
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
> +/* 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;
Kees and Christoph,
thanks for the patch and review.
On Tue, May 10, 2022 at 9:29 AM Christoph Hellwig <[email protected]> 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
Kees,
On Tue, May 10, 2022 at 5:51 PM Kees Cook <[email protected]> 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
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 <[email protected]> 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 <[email protected]>
Thanks!
--
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 <[email protected]> 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