Messages in this thread | | | From | David Laight <> | Subject | RE: new __write_overflow_field compiler warning | Date | Wed, 18 May 2022 08:57:02 +0000 |
| |
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)
| |