new __write_overflow_field compiler warning

new __write_overflow_field compiler warning

[Jeff Layton]

Hi Kees,

I'm hoping you can help with this. I recently updated to Fedora 36,
which has gcc v12, and I've started seeing this warning pop up when
compiling the ceph.ko:

In file included from ./include/linux/string.h:253,
                 from ./include/linux/ceph/ceph_debug.h:7,
                 from fs/ceph/inode.c:2:
In function ‘fortify_memset_chk’,
    inlined from ‘netfs_i_context_init’ at ./include/linux/netfs.h:326:2,
    inlined from ‘ceph_alloc_inode’ at fs/ceph/inode.c:463:2:
./include/linux/fortify-string.h:242:25: warning: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
  242 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This doesn't seem to happen with gcc v11. It looks like the code is
doing the right thing. Is there something we need to fix how the netfs
context gets initialized or is this a compiler problem?

FWIW: I'm using:

    gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1)

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: new __write_overflow_field compiler warning

[Kees Cook]

On Tue, May 17, 2022 at 02:03:52PM -0400, Jeff Layton wrote:
> Hi Kees,

Hi!

> I'm hoping you can help with this. I recently updated to Fedora 36,
> which has gcc v12, and I've started seeing this warning pop up when
> compiling the ceph.ko:
> 
> In file included from ./include/linux/string.h:253,
>                  from ./include/linux/ceph/ceph_debug.h:7,
>                  from fs/ceph/inode.c:2:
> In function ‘fortify_memset_chk’,
>     inlined from ‘netfs_i_context_init’ at ./include/linux/netfs.h:326:2,
>     inlined from ‘ceph_alloc_inode’ at fs/ceph/inode.c:463:2:
> ./include/linux/fortify-string.h:242:25: warning: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
>   242 |                         __write_overflow_field(p_size_field, size);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This doesn't seem to happen with gcc v11. It looks like the code is
> doing the right thing. Is there something we need to fix how the netfs
> context gets initialized or is this a compiler problem?
> 
> FWIW: I'm using:
> 
>     gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1)

Yeah, GCC 12 got "smarter" about how deeply it can analyze object sizes.
Usually, this has been helpful. Other times, it's a bit weirder, like
here.

So this is resolving to:

static inline void netfs_i_context_init(struct inode *inode,
                                        const struct netfs_request_ops *ops)
{
        struct netfs_i_context *ctx = netfs_i_context(inode);

        memset(ctx, 0, sizeof(*ctx));
...

In the sense that the compiler is having trouble understanding this
object, it's due to the same "unexpected" manipulations that manifest in
other areas (randstruct) which got fixed recently:
https://lore.kernel.org/lkml/20220503205503.3054173-2-keescook@chromium.org/

But it seems randstruct is happy to look the other way here after the
(void *) cast, where as __builtin_object_size() (the work-horse of the
memcpy checking) is not. Hmpf.

Ignoring the linked change above (which doesn't change the warning
here), GCC is effectively seeing:

static inline void netfs_i_context_init(struct inode *inode,
                                        const struct netfs_request_ops *ops)
{
	struct netfs_i_context *ctx = (struct netfs_i_context *)(inode + 1);

	if (__builtin_object_size(ctx, 1) < sizeof(*ctx))
		__write_overflow_field(...)

And __builtin_object_size() see "ctx" as pointing past the end of a single
"struct inode" (i.e. there are zero bytes left in the original
structure).

However, I think we can solve both the FORTIFY and the randstruct
concerns by wrapping the conversions in container_of(). This passes for
me with -next (i.e. on top of the above linked change):

diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 0c33b715cbfd..cce5a9b53a8a 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -286,6 +286,17 @@ extern void netfs_put_subrequest(struct netfs_io_subrequest *subreq,
 				 bool was_async, enum netfs_sreq_ref_trace what);
 extern void netfs_stats_show(struct seq_file *);
 
+/*
+ * The struct netfs_i_context instance must always follow the VFS inode,
+ * but existing users want to avoid a substructure name space, so just
+ * use this internally to perform the needed container_of() offset
+ * casting, which will keep both FORTIFY_SOURCE and randstruct happy.
+ */
+struct netfs_i_c_pair {
+	struct inode inode;
+	struct netfs_i_context ctx;
+};
+
 /**
  * netfs_i_context - Get the netfs inode context from the inode
  * @inode: The inode to query
@@ -295,7 +306,7 @@ extern void netfs_stats_show(struct seq_file *);
  */
 static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
 {
-	return (void *)inode + sizeof(*inode);
+	return &container_of(inode, struct netfs_i_c_pair, inode)->ctx;
 }
 
 /**
@@ -307,7 +318,7 @@ static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
  */
 static inline struct inode *netfs_inode(struct netfs_i_context *ctx)
 {
-	return (void *)ctx - sizeof(struct inode);
+	return &container_of(ctx, struct netfs_i_c_pair, ctx)->inode;
 }
 
 /**






-- 
Kees Cook

RE: new __write_overflow_field compiler warning

[David Laight]

From: Kees Cook
> Sent: 17 May 2022 21:54
> 
> On Tue, May 17, 2022 at 02:03:52PM -0400, Jeff Layton wrote:
> > Hi Kees,
> 
> Hi!
> 
> > I'm hoping you can help with this. I recently updated to Fedora 36,
> > which has gcc v12, and I've started seeing this warning pop up when
> > compiling the ceph.ko:
> >
> > In file included from ./include/linux/string.h:253,
> >                  from ./include/linux/ceph/ceph_debug.h:7,
> >                  from fs/ceph/inode.c:2:
> > In function ‘fortify_memset_chk’,
> >     inlined from ‘netfs_i_context_init’ at ./include/linux/netfs.h:326:2,
> >     inlined from ‘ceph_alloc_inode’ at fs/ceph/inode.c:463:2:
> > ./include/linux/fortify-string.h:242:25: warning: call to ‘__write_overflow_field’ declared with
> attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-
> Wattribute-warning]
> >   242 |                         __write_overflow_field(p_size_field, size);
> >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > This doesn't seem to happen with gcc v11. It looks like the code is
> > doing the right thing. Is there something we need to fix how the netfs
> > context gets initialized or is this a compiler problem?
> >
> > FWIW: I'm using:
> >
> >     gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1)
> 
> Yeah, GCC 12 got "smarter" about how deeply it can analyze object sizes.
> Usually, this has been helpful. Other times, it's a bit weirder, like
> here.
> 
> So this is resolving to:
> 
> static inline void netfs_i_context_init(struct inode *inode,
>                                         const struct netfs_request_ops *ops)
> {
>         struct netfs_i_context *ctx = netfs_i_context(inode);
> 
>         memset(ctx, 0, sizeof(*ctx));
> ...
> 
> In the sense that the compiler is having trouble understanding this
> object, it's due to the same "unexpected" manipulations that manifest in
> other areas (randstruct) which got fixed recently:
> https://lore.kernel.org/lkml/20220503205503.3054173-2-keescook@chromium.org/
> 
> But it seems randstruct is happy to look the other way here after the
> (void *) cast, where as __builtin_object_size() (the work-horse of the
> memcpy checking) is not. Hmpf.
> 
> Ignoring the linked change above (which doesn't change the warning
> here), GCC is effectively seeing:
> 
> static inline void netfs_i_context_init(struct inode *inode,
>                                         const struct netfs_request_ops *ops)
> {
> 	struct netfs_i_context *ctx = (struct netfs_i_context *)(inode + 1);
> 
> 	if (__builtin_object_size(ctx, 1) < sizeof(*ctx))
> 		__write_overflow_field(...)
> 
> And __builtin_object_size() see "ctx" as pointing past the end of a single
> "struct inode" (i.e. there are zero bytes left in the original
> structure).
> 
> However, I think we can solve both the FORTIFY and the randstruct
> concerns by wrapping the conversions in container_of(). This passes for
> me with -next (i.e. on top of the above linked change):
> 
> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index 0c33b715cbfd..cce5a9b53a8a 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -286,6 +286,17 @@ extern void netfs_put_subrequest(struct netfs_io_subrequest *subreq,
>  				 bool was_async, enum netfs_sreq_ref_trace what);
>  extern void netfs_stats_show(struct seq_file *);
> 
> +/*
> + * The struct netfs_i_context instance must always follow the VFS inode,
> + * but existing users want to avoid a substructure name space, so just
> + * use this internally to perform the needed container_of() offset
> + * casting, which will keep both FORTIFY_SOURCE and randstruct happy.
> + */
> +struct netfs_i_c_pair {
> +	struct inode inode;
> +	struct netfs_i_context ctx;
> +};
> +
>  /**
>   * netfs_i_context - Get the netfs inode context from the inode
>   * @inode: The inode to query
> @@ -295,7 +306,7 @@ extern void netfs_stats_show(struct seq_file *);
>   */
>  static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
>  {
> -	return (void *)inode + sizeof(*inode);
> +	return &container_of(inode, struct netfs_i_c_pair, inode)->ctx;
>  }
> 
>  /**
> @@ -307,7 +318,7 @@ static inline struct netfs_i_context *netfs_i_context(struct inode *inode)
>   */
>  static inline struct inode *netfs_inode(struct netfs_i_context *ctx)
>  {
> -	return (void *)ctx - sizeof(struct inode);
> +	return &container_of(ctx, struct netfs_i_c_pair, ctx)->inode;
>  }
> 
>  /**

That is unreadable crap.
Can't the compiler be fixed so that it doesn't object to
a very common construct.

Are you sure the compiler isn't returning a size of 0
when it doesn't know the size - as well as when it knows the
size is 0.
Which would mean that all the checks in the kernel headers
are just wrong.

is it enough to replace:
> 	struct netfs_i_context *ctx = (struct netfs_i_context *)(inode + 1);
with
	ctx = (void *)(long)(inode + 1);
or:
	ctx = (void *)((long)inode + sizeof *inode);

Failing that add struct_after() and struct_before()
definitions somewhere.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)