netfs: Use container_of() for offset casting

Commit Message

Kees Cook May 17, 2022, 9:02 p.m. UTC

While randstruct was satisfied with using an open-coded "void *" offset
cast for the netfs_i_context <-> inode casting, __builtin_object_size()
as used by FORTIFY_SOURCE was not as easily fooled. Switch to using
an internally defined netfs_i_context/inode struct for doing a full
container_of() casting. This keeps both randstruct and __bos() happy
under GCC 12. Silences:

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);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Jeff Layton <[e-mail]>
Link: https://lore.kernel.org/lkml/[e-mail]
Cc: Jeff Layton <[e-mail]>
Cc: David Howells <[e-mail]>
Signed-off-by: Kees Cook <[e-mail]>
---
If this looks good I can add it to my hardening tree, or if you want to
carry it, I can respin this without the earlier randstruct changes and
drop that patch from my tree?
---
 include/linux/netfs.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Jeff Layton May 17, 2022, 10:32 p.m. UTC | #1

On Tue, 2022-05-17 at 14:02 -0700, Kees Cook wrote:
> While randstruct was satisfied with using an open-coded "void *" offset
> cast for the netfs_i_context <-> inode casting, __builtin_object_size()
> as used by FORTIFY_SOURCE was not as easily fooled. Switch to using
> an internally defined netfs_i_context/inode struct for doing a full
> container_of() casting. This keeps both randstruct and __bos() happy
> under GCC 12. Silences:
> 
> 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);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Jeff Layton <[e-mail]>
> Link: https://lore.kernel.org/lkml/[e-mail]
> Cc: Jeff Layton <[e-mail]>
> Cc: David Howells <[e-mail]>
> Signed-off-by: Kees Cook <[e-mail]>
> ---
> If this looks good I can add it to my hardening tree, or if you want to
> carry it, I can respin this without the earlier randstruct changes and
> drop that patch from my tree?
> ---
>  include/linux/netfs.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> 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;
>  }
>  
>  /**

This patch didn't apply cleanly for me to a recent tree, but I was able
to wiggle it into place and it seemed to work.

Tested-by: Jeff Layton <[e-mail]>

David Howells May 18, 2022, 8:05 a.m. UTC | #2

I wonder if it would be worth making this explicit in the inode wrappers of
the users of netfslib.  In afs, for instance, there is:

	struct afs_vnode {
		struct {
			/* These must be contiguous */
			struct inode	vfs_inode;
			struct netfs_i_context netfs_ctx;
		};
		...
	};

would it be worth making that:

	struct afs_vnode {
		union {
			struct netfs_i_c_pair netfs_inode;
			struct {
				/* These must be contiguous */
				struct inode	vfs_inode;
				struct netfs_i_context netfs_ctx;
			};
		};
		...
	};

I don't want to do the following, say:


	struct afs_vnode {
		struct netfs_i_c_pair ni;
		...
	};

as that would then require a lot of s/->vfs_inode/->ni.vfs_inode/, but maybe
it would be better to include a struct inode in struct netfs_i_context, and
then do:

	struct afs_vnode {
		union {
			struct netfs_i_context netfs_ctx;
			struct inode vfs_inode;
		};
		...
	};

and perhaps rename netfs_i_context to netfs_inode (though that looks a bit
close to nfs_inode).  It's just a shame I can't do:

	struct netfs_inode : inode {
		...
	};

	struct afs_vnode : netfs_inode {
		...
	};

right? ;-)

On the other hand:

	warthog>git grep '[>.]vfs_inode' -- fs/{9p,afs,ceph,cifs,nfs} | wc -l
	181

so maybe a mass change to, say:

	struct netfs_inode {
		struct inode vfs_inode;
		...
	};

	struct afs_vnode {
		struct netfs_inode ni;
		...
	};

wouldn't be so bad.

David

David Laight May 18, 2022, 10:01 a.m. UTC | #3

From: David Howells 
> Sent: 18 May 2022 09:05
> 
> I wonder if it would be worth making this explicit in the inode wrappers of
> the users of netfslib.  In afs, for instance, there is:
> 
> 	struct afs_vnode {
> 		struct {
> 			/* These must be contiguous */
> 			struct inode	vfs_inode;
> 			struct netfs_i_context netfs_ctx;
> 		};
> 		...
> 	};
> 
> would it be worth making that:
> 
> 	struct afs_vnode {
> 		union {
> 			struct netfs_i_c_pair netfs_inode;
> 			struct {
> 				/* These must be contiguous */
> 				struct inode	vfs_inode;
> 				struct netfs_i_context netfs_ctx;
> 			};
> 		};
> 		...
> 	};
> 

Can't you just name the structure so it is:

	struct afs_vnode {
 		struct netfs_i_c_pair {
 			/* These must be contiguous */
 			struct inode	vfs_inode;
 			struct netfs_i_context netfs_ctx;
 		};
 		...
 	};

	David

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

David Howells May 18, 2022, 3:21 p.m. UTC | #4

David Laight <[e-mail]> wrote:

> Can't you just name the structure so it is:
> 
> 	struct afs_vnode {
>  		struct netfs_i_c_pair {
>  			/* These must be contiguous */
>  			struct inode	vfs_inode;
>  			struct netfs_i_context netfs_ctx;
>  		};
>  		...
>  	};

No.  C won't let you do that (the same thing has to be done in 9p, ceph as
well, and at some point hopefully cifs and nfs too).

David

Kees Cook May 18, 2022, 8:21 p.m. UTC | #5

On Wed, May 18, 2022 at 09:05:14AM +0100, David Howells wrote:
> [...]
> I don't want to do the following, say:
> 
> 
> 	struct afs_vnode {
> 		struct netfs_i_c_pair ni;
> 		...
> 	};
> 
> as that would then require a lot of s/->vfs_inode/->ni.vfs_inode/, but maybe
> it would be better to include a struct inode in struct netfs_i_context, and

Right; that's why I kept the struct internal -- the implicit ordering of
inode and netfs_i_context is already present in all the users.

> On the other hand:
> 
> 	warthog>git grep '[>.]vfs_inode' -- fs/{9p,afs,ceph,cifs,nfs} | wc -l
> 	181

That seems painful. Maybe _new_ users of netfs could be written to use
the proposed netfs_inode:

> so maybe a mass change to, say:
> 
> 	struct netfs_inode {
> 		struct inode vfs_inode;
> 		...
> 	};

Better yet, netfs can define a macro helper. I'll send a v2...

Patch

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;
 }
 
 /**