Message-ID: <20220524101205.GI2306852@dread.disaster.area>
Date:   Tue, 24 May 2022 20:12:05 +1000
From:   Dave Chinner <david@...morbit.com>
To:     David Howells <dhowells@...hat.com>
Cc:     jlayton@...nel.org, keescook@...omium.org,
        Jonathan Corbet <corbet@....net>,
        Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        Dominique Martinet <asmadeus@...ewreck.org>,
        Christian Schoenebeck <linux_oss@...debyte.com>,
        Marc Dionne <marc.dionne@...istor.com>,
        Xiubo Li <xiubli@...hat.com>,
        Ilya Dryomov <idryomov@...il.com>,
        Steve French <smfrench@...il.com>,
        William Kucharski <william.kucharski@...cle.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        linux-doc@...r.kernel.org, v9fs-developer@...ts.sourceforge.net,
        linux-afs@...ts.infradead.org, ceph-devel@...r.kernel.org,
        linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
        linux-fsdevel@...r.kernel.org, linux-hardening@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] netfs: Fix gcc-12 warning by embedding vfs inode in
 netfs_i_context

On Fri, May 20, 2022 at 03:47:36PM +0100, David Howells 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.  This was causing the
> following complaint[1] from gcc v12:
> 
> 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);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fix this by embedding a struct inode into struct netfs_i_context (which
> should perhaps be renamed to struct netfs_inode).  The struct inode
> vfs_inode fields are then removed from the 9p, afs, ceph and cifs inode
> structs and vfs_inode is then simply changed to "netfs.inode" in those
> filesystems.
> 
> Further, rename netfs_i_context to netfs_inode, get rid of the
> netfs_inode() function that converted a netfs_i_context pointer to an inode
> pointer (that can now be done with &ctx->inode) and rename the
> netfs_i_context() function to netfs_inode() (which is now a wrapper around
> container_of()).
> 
> Most of the changes were done with:
> 
>   perl -p -i -e 's/vfs_inode/netfs.inode/'g \
> 	`git grep -l 'vfs_inode' -- fs/{9p,afs,ceph,cifs}/*.[ch]`
> 
> Kees suggested doing it with a pair structure[2] and a special declarator
> to insert that into the network filesystem's inode wrapper[3], but I think
> it's cleaner to embed it - and then it doesn't matter if struct
> randomisation reorders things.

I can't help but think the code would be so much cleaner with a
wrapper to covert from the filesysetm structure to the vfs inode.
e.g. in XFS we use VFS_I(xfs_inode) to get the struct inode and
XFS_I(inode) to get the xfs_inode from the struct inode.
i.e.:

/* Convert from vfs inode to xfs inode */
static inline struct xfs_inode *XFS_I(struct inode *inode)
{
        return container_of(inode, struct xfs_inode, i_vnode);
}

/* convert from xfs inode to vfs inode */
static inline struct inode *VFS_I(struct xfs_inode *ip)
{
        return &ip->i_vnode;
}

Then we end up with stuff like this reading:

> diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
> index e28ddf763b3b..0129de2ea31a 100644
> --- a/fs/9p/v9fs.c
> +++ b/fs/9p/v9fs.c
> @@ -625,7 +625,7 @@ static void v9fs_inode_init_once(void *foo)
>  	struct v9fs_inode *v9inode = (struct v9fs_inode *)foo;
>  
>  	memset(&v9inode->qid, 0, sizeof(v9inode->qid));
> -	inode_init_once(&v9inode->vfs_inode);
> +	inode_init_once(&v9inode->netfs.inode);

	inode_init_once(VFS_I(v9inode));

>  }
>  
>  /**
> diff --git a/fs/9p/v9fs.h b/fs/9p/v9fs.h
> index ec0e8df3b2eb..1b219c21d15e 100644
> --- a/fs/9p/v9fs.h
> +++ b/fs/9p/v9fs.h
> @@ -109,11 +109,7 @@ struct v9fs_session_info {
>  #define V9FS_INO_INVALID_ATTR 0x01
>  
>  struct v9fs_inode {
> -	struct {
> -		/* These must be contiguous */
> -		struct inode	vfs_inode;	/* the VFS's inode record */
> -		struct netfs_i_context netfs_ctx; /* Netfslib context */
> -	};
> +	struct netfs_inode netfs; /* Netfslib context and vfs inode */
>  	struct p9_qid qid;
>  	unsigned int cache_validity;
>  	struct p9_fid *writeback_fid;
> @@ -122,13 +118,13 @@ struct v9fs_inode {
>  
>  static inline struct v9fs_inode *V9FS_I(const struct inode *inode)
>  {
> -	return container_of(inode, struct v9fs_inode, vfs_inode);
> +	return container_of(inode, struct v9fs_inode, netfs.inode);
>  }

Looky dat - there's already the V9FS_I() function for going from the
VFS inode to the 9p inode....

I think that having a VFS_I() for every filesystem would make all
this code a lot cleaner, and it would be easier for everyone to
understand without having to know the exact details of how the netfs
inode encapsulates the struct inode. Consistency of code conventions
across multiple filesystems is a good thing. And if this netfs inode
structure ever has to be changed in future, it's just a few wrapper
functions that need updating, not lots of code...

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists