Re: CVS commit: src

[Andrew Doran <ad%netbsd.org@localhost>]

On Fri, Feb 21, 2020 at 02:14:31PM +0100, Kamil Rytarowski wrote:

> On 22.12.2019 20:47, Andrew Doran wrote:
> > Module Name:	src
> > Committed By:	ad
> > Date:		Sun Dec 22 19:47:35 UTC 2019
> > 
> > Modified Files:
> > 	src/external/cddl/osnet/dist/uts/common/fs/zfs: zfs_ctldir.c
> > 	src/sys/kern: vfs_mount.c vfs_subr.c vfs_syscalls.c
> > 	src/sys/miscfs/genfs: genfs_vfsops.c
> > 	src/sys/nfs: nfs_export.c
> > 	src/sys/sys: mount.h vnode.h vnode_impl.h
> > 	src/sys/ufs/lfs: ulfs_vfsops.c
> > 	src/sys/ufs/ufs: ufs_vfsops.c ufs_wapbl.c
> > 
> > Log Message:
> > Make mntvnode_lock per-mount, and address false sharing of struct mount.
> > 
> 
> This change broke kUBSan syzbot.
> 
> The sanitizer is now very noisy as struct mount requires 64 byte alignment.
> 
> http://netbsd.org/~kamil/kubsan/mount-alignment.txt

I had a look this weekend.  This is down to KMEM_SIZE messing up the
alignment, so is a DIAGNOSTIC thing.  The align_offset parameter to
pool_cache() would be a nice easy way to solve this but it seems someone
killed that off, so I'll need to give this some more thought.

Andrew

> > diff -u src/sys/sys/mount.h:1.234 src/sys/sys/mount.h:1.235
> > --- src/sys/sys/mount.h:1.234	Tue Jan  1 10:06:54 2019
> > +++ src/sys/sys/mount.h	Sun Dec 22 19:47:34 2019
> > @@ -1,4 +1,4 @@
> > -/*	$NetBSD: mount.h,v 1.234 2019/01/01 10:06:54 hannken Exp $	*/
> > +/*	$NetBSD: mount.h,v 1.235 2019/12/22 19:47:34 ad Exp $	*/
> >  
> >  /*
> >   * Copyright (c) 1989, 1991, 1993
> > @@ -133,29 +133,38 @@ struct vattr;
> >   * array of operations and an instance record.
> >   */
> >  struct mount {
> > -	TAILQ_HEAD(, vnode_impl) mnt_vnodelist;	/* list of vnodes this mount */
> > +	/*
> > +	 * Mostly stable data.
> > +	 */
> > +	kmutex_t	*mnt_vnodelock;		/* lock on mnt_vnodelist */
> >  	struct vfsops	*mnt_op;		/* operations on fs */
> >  	struct vnode	*mnt_vnodecovered;	/* vnode we mounted on */
> >  	struct mount	*mnt_lower;		/* fs mounted on */
> > -	int		mnt_synclist_slot;	/* synclist slot index */
> >  	void		*mnt_transinfo;		/* for FS-internal use */
> >  	void		*mnt_data;		/* private data */
> > -	kmutex_t	mnt_renamelock;		/* per-fs rename lock */
> > -	int		mnt_refcnt;		/* ref count on this structure */
> > +	kmutex_t	*mnt_renamelock;	/* per-fs rename lock */
> >  	int		mnt_flag;		/* flags */
> >  	int		mnt_iflag;		/* internal flags */
> >  	int		mnt_fs_bshift;		/* offset shift for lblkno */
> >  	int		mnt_dev_bshift;		/* shift for device sectors */
> > -	struct statvfs	mnt_stat;		/* cache of filesystem stats */
> >  	specificdata_reference
> >  			mnt_specdataref;	/* subsystem specific data */
> > -	kmutex_t	mnt_updating;		/* to serialize updates */
> > +	kmutex_t	*mnt_updating;		/* to serialize updates */
> >  	const struct wapbl_ops
> >  			*mnt_wapbl_op;		/* logging ops */
> >  	struct wapbl	*mnt_wapbl;		/* log info */
> >  	struct wapbl_replay
> >  			*mnt_wapbl_replay;	/* replay support XXX: what? */
> >  	uint64_t	mnt_gen;
> > +
> > +	/*
> > +	 * Volatile data: pad to keep away from the stable items.
> > +	 */
> > +	int		mnt_refcnt		/* ref count on this structure */
> > +	    __aligned(COHERENCY_UNIT);
> > +	int		mnt_synclist_slot;	/* synclist slot index */
> > +	TAILQ_HEAD(, vnode_impl) mnt_vnodelist;	/* list of vnodes this mount */
> > +	struct statvfs	mnt_stat;		/* cache of filesystem stats */
> >  };
> >  
> >  #endif /* defined(_KERNEL) || defined(__EXPOSE_MOUNT) */
> > 
> 
> 
> 
>