Re: [PATCH 13/29] xfs: add fs-verity support

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Apr 02, 2024 at 10:42:44AM +0200, Andrey Albershteyn wrote:
> On 2024-03-29 17:39:27, Darrick J. Wong wrote:
> > From: Andrey Albershteyn <aalbersh@redhat.com>
> > 
> > Add integration with fs-verity. The XFS store fs-verity metadata in
> > the extended file attributes. The metadata consist of verity
> > descriptor and Merkle tree blocks.
> > 
> > The descriptor is stored under "vdesc" extended attribute. The
> > Merkle tree blocks are stored under binary indexes which are offsets
> > into the Merkle tree.
> > 
> > When fs-verity is enabled on an inode, the XFS_IVERITY_CONSTRUCTION
> > flag is set meaning that the Merkle tree is being build. The
> > initialization ends with storing of verity descriptor and setting
> > inode on-disk flag (XFS_DIFLAG2_VERITY).
> > 
> > The verification on read is done in read path of iomap.
> > 
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > [djwong: replace caching implementation with an xarray, other cleanups]
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/Makefile               |    2 
> >  fs/xfs/libxfs/xfs_attr.c      |   41 +++
> >  fs/xfs/libxfs/xfs_attr.h      |    1 
> >  fs/xfs/libxfs/xfs_da_format.h |   14 +
> >  fs/xfs/libxfs/xfs_ondisk.h    |    3 
> >  fs/xfs/libxfs/xfs_verity.c    |   58 ++++
> >  fs/xfs/libxfs/xfs_verity.h    |   13 +
> >  fs/xfs/xfs_fsverity.c         |  559 +++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/xfs_fsverity.h         |   20 +
> >  fs/xfs/xfs_icache.c           |    4 
> >  fs/xfs/xfs_inode.h            |    5 
> >  fs/xfs/xfs_super.c            |   17 +
> >  fs/xfs/xfs_trace.h            |   32 ++
> >  13 files changed, 769 insertions(+)
> >  create mode 100644 fs/xfs/libxfs/xfs_verity.c
> >  create mode 100644 fs/xfs/libxfs/xfs_verity.h
> >  create mode 100644 fs/xfs/xfs_fsverity.c
> >  create mode 100644 fs/xfs/xfs_fsverity.h
> > 
> > 

<snip>

> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index 5a202706fc4a4..70c5700132b3e 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -96,6 +96,9 @@ typedef struct xfs_inode {
> >  	spinlock_t		i_ioend_lock;
> >  	struct work_struct	i_ioend_work;
> >  	struct list_head	i_ioend_list;
> > +#ifdef CONFIG_FS_VERITY
> > +	struct xarray		i_merkle_blocks;
> > +#endif
> 
> So, is this fine like this or do you plan to change it to per-ag
> mapping? I suppose Christoph against adding it to inodes [1]
> 
> [1]: https://lore.kernel.org/linux-xfs/ZfecSzBoVDW5328l@infradead.org/

Still working on it.  hch and I have been nitpicking the parent pointers
patchset.  I think a per-ag rhashtable would work in principle, but I
don't know how well it will handle a 128-bit key.

--D

> >  } xfs_inode_t;
> >  
> >  static inline bool xfs_inode_on_unlinked_list(const struct xfs_inode *ip)
> > @@ -391,6 +394,8 @@ static inline bool xfs_inode_needs_cow_around(struct xfs_inode *ip)
> >   */
> >  #define XFS_IREMAPPING		(1U << 15)
> >  
> > +#define XFS_VERITY_CONSTRUCTION	(1U << 16) /* merkle tree construction */
> > +
> >  /* All inode state flags related to inode reclaim. */
> >  #define XFS_ALL_IRECLAIM_FLAGS	(XFS_IRECLAIMABLE | \
> >  				 XFS_IRECLAIM | \
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 42a1e1f23d3b3..4e398884c46ae 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -30,6 +30,7 @@
> >  #include "xfs_filestream.h"
> >  #include "xfs_quota.h"
> >  #include "xfs_sysfs.h"
> > +#include "xfs_fsverity.h"
> >  #include "xfs_ondisk.h"
> >  #include "xfs_rmap_item.h"
> >  #include "xfs_refcount_item.h"
> > @@ -53,6 +54,7 @@
> >  #include <linux/fs_context.h>
> >  #include <linux/fs_parser.h>
> >  #include <linux/fsverity.h>
> > +#include <linux/iomap.h>
> >  
> >  static const struct super_operations xfs_super_operations;
> >  
> > @@ -672,6 +674,8 @@ xfs_fs_destroy_inode(
> >  	ASSERT(!rwsem_is_locked(&inode->i_rwsem));
> >  	XFS_STATS_INC(ip->i_mount, vn_rele);
> >  	XFS_STATS_INC(ip->i_mount, vn_remove);
> > +	if (fsverity_active(inode))
> > +		xfs_fsverity_cache_drop(ip);
> >  	fsverity_cleanup_inode(inode);
> >  	xfs_inode_mark_reclaimable(ip);
> >  }
> > @@ -1534,6 +1538,9 @@ xfs_fs_fill_super(
> >  	sb->s_quota_types = QTYPE_MASK_USR | QTYPE_MASK_GRP | QTYPE_MASK_PRJ;
> >  #endif
> >  	sb->s_op = &xfs_super_operations;
> > +#ifdef CONFIG_FS_VERITY
> > +	sb->s_vop = &xfs_fsverity_ops;
> > +#endif
> >  
> >  	/*
> >  	 * Delay mount work if the debug hook is set. This is debug
> > @@ -1775,10 +1782,20 @@ xfs_fs_fill_super(
> >  		xfs_warn(mp,
> >  	"EXPERIMENTAL parent pointer feature enabled. Use at your own risk!");
> >  
> > +	if (xfs_has_verity(mp))
> > +		xfs_alert(mp,
> > +	"EXPERIMENTAL fsverity feature in use. Use at your own risk!");
> > +
> >  	error = xfs_mountfs(mp);
> >  	if (error)
> >  		goto out_filestream_unmount;
> >  
> > +#ifdef CONFIG_FS_VERITY
> > +	error = iomap_init_fsverity(mp->m_super);
> > +	if (error)
> > +		goto out_unmount;
> > +#endif
> > +
> >  	root = igrab(VFS_I(mp->m_rootip));
> >  	if (!root) {
> >  		error = -ENOENT;
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index e2992b0115ad2..86a8702c1e27c 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -5908,6 +5908,38 @@ TRACE_EVENT(xfs_growfs_check_rtgeom,
> >  );
> >  #endif /* CONFIG_XFS_RT */
> >  
> > +#ifdef CONFIG_FS_VERITY
> > +DECLARE_EVENT_CLASS(xfs_fsverity_cache_class,
> > +	TP_PROTO(struct xfs_inode *ip, unsigned long key, unsigned long caller_ip),
> > +	TP_ARGS(ip, key, caller_ip),
> > +	TP_STRUCT__entry(
> > +		__field(dev_t, dev)
> > +		__field(xfs_ino_t, ino)
> > +		__field(unsigned long, key)
> > +		__field(void *, caller_ip)
> > +	),
> > +	TP_fast_assign(
> > +		__entry->dev = ip->i_mount->m_super->s_dev;
> > +		__entry->ino = ip->i_ino;
> > +		__entry->key = key;
> > +		__entry->caller_ip = (void *)caller_ip;
> > +	),
> > +	TP_printk("dev %d:%d ino 0x%llx key 0x%lx caller %pS",
> > +		  MAJOR(__entry->dev), MINOR(__entry->dev),
> > +		  __entry->ino,
> > +		  __entry->key,
> > +		  __entry->caller_ip)
> > +)
> > +
> > +#define DEFINE_XFS_FSVERITY_CACHE_EVENT(name) \
> > +DEFINE_EVENT(xfs_fsverity_cache_class, name, \
> > +	TP_PROTO(struct xfs_inode *ip, unsigned long key, unsigned long caller_ip), \
> > +	TP_ARGS(ip, key, caller_ip))
> > +DEFINE_XFS_FSVERITY_CACHE_EVENT(xfs_fsverity_cache_load);
> > +DEFINE_XFS_FSVERITY_CACHE_EVENT(xfs_fsverity_cache_store);
> > +DEFINE_XFS_FSVERITY_CACHE_EVENT(xfs_fsverity_cache_drop);
> > +#endif /* CONFIG_XFS_VERITY */
> > +
> >  #endif /* _TRACE_XFS_H */
> >  
> >  #undef TRACE_INCLUDE_PATH
> > 
> 
> -- 
> - Andrey
> 
>