[PATCH 0/5] xfs: general scrubber fixes

[PATCH 0/5] xfs: general scrubber fixes

[Darrick J. Wong]

Hi all,

Here's a second series of scrubber fixes that apply elsewhere.

Patch 1 aborts the xattr scrub loop if there are pending fatal signals.

Patch 2 checks that for any directory or attr fork there are no extent
maps that stretch beyond what a xfs_dablk_t can map.

Patch 3 fixes an off-by-one error in a rtbitmap scrub helper.

Patches 4-5 implement name check functions for directory entries and
extended attribute leaves.  For now they're only used by scrub (and used
to clean up xfs_repair) but I suspect it might be more appropriate to
put them in the regular verifiers.

If you're going to start using this mess, you probably ought to just
pull from my git trees.  The kernel patches[1] should apply against
5.0-rc1.

Comments and questions are, as always, welcome.

--D

[PATCH 1/5] xfs: abort xattr scrub if fatal signals are pending

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The extended attribute scrubber should abort the "read all attrs" loop
if there's a fatal signal pending on the process.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/scrub/attr.c |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index 81d5e90547a1..9960bc5b5d76 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -82,6 +82,11 @@ xchk_xattr_listent(
 
 	sx = container_of(context, struct xchk_xattr, context);
 
+	if (xchk_should_terminate(sx->sc, &error)) {
+		context->seen_enough = 1;
+		return;
+	}
+
 	if (flags & XFS_ATTR_INCOMPLETE) {
 		/* Incomplete attr key, just mark the inode for preening. */
 		xchk_ino_set_preen(sx->sc, context->dp->i_ino);

[PATCH 2/5] xfs: scrub should flag dir/attr offsets that aren't mappable with xfs_dablk_t

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Teach scrub to flag extent maps that exceed the range that can be mapped
with a xfs_dablk_t.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_types.c |   11 +++++++++++
 fs/xfs/libxfs/xfs_types.h |    1 +
 fs/xfs/scrub/bmap.c       |   27 +++++++++++++++++++++++++++
 3 files changed, 39 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
index 3306fc42cfad..451608b5a37b 100644
--- a/fs/xfs/libxfs/xfs_types.c
+++ b/fs/xfs/libxfs/xfs_types.c
@@ -204,3 +204,14 @@ xfs_verify_icount(
 	xfs_icount_range(mp, &min, &max);
 	return icount >= min && icount <= max;
 }
+
+/* Sanity-checking of dir/attr block offsets. */
+bool
+xfs_verify_dablk(
+	struct xfs_mount	*mp,
+	xfs_fileoff_t		dabno)
+{
+	xfs_dablk_t		max_dablk = -1U;
+
+	return dabno <= max_dablk;
+}
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 8f02855a019a..704b4f308780 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -188,5 +188,6 @@ bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
 bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
+bool xfs_verify_dablk(struct xfs_mount *mp, xfs_fileoff_t off);
 
 #endif	/* __XFS_TYPES_H__ */
diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index e1d11f3223e3..a703cd58a90e 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -281,6 +281,31 @@ xchk_bmap_extent_xref(
 	xchk_ag_free(info->sc, &info->sc->sa);
 }
 
+/*
+ * Directories and attr forks should never have blocks that can't be addressed
+ * by a xfs_dablk_t.
+ */
+STATIC void
+xchk_bmap_dirattr_extent(
+	struct xfs_inode	*ip,
+	struct xchk_bmap_info	*info,
+	struct xfs_bmbt_irec	*irec)
+{
+	struct xfs_mount	*mp = ip->i_mount;
+	xfs_fileoff_t		off;
+
+	if (!S_ISDIR(VFS_I(ip)->i_mode) && info->whichfork != XFS_ATTR_FORK)
+		return;
+
+	if (!xfs_verify_dablk(mp, irec->br_startoff))
+		xchk_fblock_set_corrupt(info->sc, info->whichfork,
+				irec->br_startoff);
+
+	off = irec->br_startoff + irec->br_blockcount - 1;
+	if (!xfs_verify_dablk(mp, off))
+		xchk_fblock_set_corrupt(info->sc, info->whichfork, off);
+}
+
 /* Scrub a single extent record. */
 STATIC int
 xchk_bmap_extent(
@@ -305,6 +330,8 @@ xchk_bmap_extent(
 		xchk_fblock_set_corrupt(info->sc, info->whichfork,
 				irec->br_startoff);
 
+	xchk_bmap_dirattr_extent(ip, info, irec);
+
 	/* There should never be a "hole" extent in either extent list. */
 	if (irec->br_startblock == HOLESTARTBLOCK)
 		xchk_fblock_set_corrupt(info->sc, info->whichfork,

Re: [PATCH 2/5] xfs: scrub should flag dir/attr offsets that aren't mappable with xfs_dablk_t

[Brian Foster]

On Tue, Jan 08, 2019 at 12:36:17PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach scrub to flag extent maps that exceed the range that can be mapped
> with a xfs_dablk_t.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_types.c |   11 +++++++++++
>  fs/xfs/libxfs/xfs_types.h |    1 +
>  fs/xfs/scrub/bmap.c       |   27 +++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> index 3306fc42cfad..451608b5a37b 100644
> --- a/fs/xfs/libxfs/xfs_types.c
> +++ b/fs/xfs/libxfs/xfs_types.c
> @@ -204,3 +204,14 @@ xfs_verify_icount(
>  	xfs_icount_range(mp, &min, &max);
>  	return icount >= min && icount <= max;
>  }
> +
> +/* Sanity-checking of dir/attr block offsets. */
> +bool
> +xfs_verify_dablk(
> +	struct xfs_mount	*mp,
> +	xfs_fileoff_t		dabno)
> +{
> +	xfs_dablk_t		max_dablk = -1U;
> +
> +	return dabno <= max_dablk;
> +}
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 8f02855a019a..704b4f308780 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -188,5 +188,6 @@ bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
>  bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
> +bool xfs_verify_dablk(struct xfs_mount *mp, xfs_fileoff_t off);
>  
>  #endif	/* __XFS_TYPES_H__ */
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index e1d11f3223e3..a703cd58a90e 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -281,6 +281,31 @@ xchk_bmap_extent_xref(
>  	xchk_ag_free(info->sc, &info->sc->sa);
>  }
>  
> +/*
> + * Directories and attr forks should never have blocks that can't be addressed
> + * by a xfs_dablk_t.
> + */
> +STATIC void
> +xchk_bmap_dirattr_extent(
> +	struct xfs_inode	*ip,
> +	struct xchk_bmap_info	*info,
> +	struct xfs_bmbt_irec	*irec)
> +{
> +	struct xfs_mount	*mp = ip->i_mount;
> +	xfs_fileoff_t		off;
> +
> +	if (!S_ISDIR(VFS_I(ip)->i_mode) && info->whichfork != XFS_ATTR_FORK)
> +		return;
> +
> +	if (!xfs_verify_dablk(mp, irec->br_startoff))
> +		xchk_fblock_set_corrupt(info->sc, info->whichfork,
> +				irec->br_startoff);
> +
> +	off = irec->br_startoff + irec->br_blockcount - 1;
> +	if (!xfs_verify_dablk(mp, off))
> +		xchk_fblock_set_corrupt(info->sc, info->whichfork, off);
> +}
> +
>  /* Scrub a single extent record. */
>  STATIC int
>  xchk_bmap_extent(
> @@ -305,6 +330,8 @@ xchk_bmap_extent(
>  		xchk_fblock_set_corrupt(info->sc, info->whichfork,
>  				irec->br_startoff);
>  
> +	xchk_bmap_dirattr_extent(ip, info, irec);
> +
>  	/* There should never be a "hole" extent in either extent list. */
>  	if (irec->br_startblock == HOLESTARTBLOCK)
>  		xchk_fblock_set_corrupt(info->sc, info->whichfork,
> 

[PATCH 3/5] xfs: fix off-by-one error in rtbitmap cross-reference

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Fix an off-by-one error in the realtime bitmap "is used" cross-reference
helper function.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/rtbitmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
index 665d4bbb17cc..5402211f980b 100644
--- a/fs/xfs/scrub/rtbitmap.c
+++ b/fs/xfs/scrub/rtbitmap.c
@@ -143,7 +143,7 @@ xchk_xref_is_used_rt_space(
 	do_div(startext, sc->mp->m_sb.sb_rextsize);
 	if (do_div(endext, sc->mp->m_sb.sb_rextsize))
 		endext++;
-	extcount = endext - startext;
+	extcount = endext - startext + 1;
 	xfs_ilock(sc->mp->m_rbmip, XFS_ILOCK_SHARED | XFS_ILOCK_RTBITMAP);
 	error = xfs_rtalloc_extent_is_free(sc->mp, sc->tp, startext, extcount,
 			&is_free);

Re: [PATCH 3/5] xfs: fix off-by-one error in rtbitmap cross-reference

[Brian Foster]

On Tue, Jan 08, 2019 at 12:36:23PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix an off-by-one error in the realtime bitmap "is used" cross-reference
> helper function.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/rtbitmap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index 665d4bbb17cc..5402211f980b 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -143,7 +143,7 @@ xchk_xref_is_used_rt_space(
>  	do_div(startext, sc->mp->m_sb.sb_rextsize);
>  	if (do_div(endext, sc->mp->m_sb.sb_rextsize))
>  		endext++;
> -	extcount = endext - startext;
> +	extcount = endext - startext + 1;

I'm not terribly familiar with rt code, but isn't the above endext++
also rounding this up in some cases?

Brian

>  	xfs_ilock(sc->mp->m_rbmip, XFS_ILOCK_SHARED | XFS_ILOCK_RTBITMAP);
>  	error = xfs_rtalloc_extent_is_free(sc->mp, sc->tp, startext, extcount,
>  			&is_free);
> 

Re: [PATCH 3/5] xfs: fix off-by-one error in rtbitmap cross-reference

[Darrick J. Wong]

On Fri, Jan 11, 2019 at 10:10:54AM -0500, Brian Foster wrote:
> On Tue, Jan 08, 2019 at 12:36:23PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Fix an off-by-one error in the realtime bitmap "is used" cross-reference
> > helper function.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/rtbitmap.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> > index 665d4bbb17cc..5402211f980b 100644
> > --- a/fs/xfs/scrub/rtbitmap.c
> > +++ b/fs/xfs/scrub/rtbitmap.c
> > @@ -143,7 +143,7 @@ xchk_xref_is_used_rt_space(
> >  	do_div(startext, sc->mp->m_sb.sb_rextsize);
> >  	if (do_div(endext, sc->mp->m_sb.sb_rextsize))
> >  		endext++;
> > -	extcount = endext - startext;
> > +	extcount = endext - startext + 1;
> 
> I'm not terribly familiar with rt code, but isn't the above endext++
> also rounding this up in some cases?

Yep.  Sorry for the drain bamage, this ought to be:

	startext = fsbno;
	endext = fsbno + len - 1;
	do_div(startext, sc->mp->m_sb.sb_rextsize);
	do_div(endext, sc->mp->m_sb.sb_rextsize);
	extcount = endext - startext + 1;

--D

> Brian
> 
> >  	xfs_ilock(sc->mp->m_rbmip, XFS_ILOCK_SHARED | XFS_ILOCK_RTBITMAP);
> >  	error = xfs_rtalloc_extent_is_free(sc->mp, sc->tp, startext, extcount,
> >  			&is_free);
> > 

[PATCH v2 3/5] xfs: fix off-by-one error in rtbitmap cross-reference

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Fix an off-by-one error in the realtime bitmap "is used" cross-reference
helper function if the realtime extent size is a single block.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/rtbitmap.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
index 665d4bbb17cc..dbe115b075f7 100644
--- a/fs/xfs/scrub/rtbitmap.c
+++ b/fs/xfs/scrub/rtbitmap.c
@@ -141,9 +141,8 @@ xchk_xref_is_used_rt_space(
 	startext = fsbno;
 	endext = fsbno + len - 1;
 	do_div(startext, sc->mp->m_sb.sb_rextsize);
-	if (do_div(endext, sc->mp->m_sb.sb_rextsize))
-		endext++;
-	extcount = endext - startext;
+	do_div(endext, sc->mp->m_sb.sb_rextsize);
+	extcount = endext - startext + 1;
 	xfs_ilock(sc->mp->m_rbmip, XFS_ILOCK_SHARED | XFS_ILOCK_RTBITMAP);
 	error = xfs_rtalloc_extent_is_free(sc->mp, sc->tp, startext, extcount,
 			&is_free);

Re: [PATCH v2 3/5] xfs: fix off-by-one error in rtbitmap cross-reference

[Brian Foster]

On Mon, Jan 14, 2019 at 12:32:01PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix an off-by-one error in the realtime bitmap "is used" cross-reference
> helper function if the realtime extent size is a single block.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/scrub/rtbitmap.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> index 665d4bbb17cc..dbe115b075f7 100644
> --- a/fs/xfs/scrub/rtbitmap.c
> +++ b/fs/xfs/scrub/rtbitmap.c
> @@ -141,9 +141,8 @@ xchk_xref_is_used_rt_space(
>  	startext = fsbno;
>  	endext = fsbno + len - 1;
>  	do_div(startext, sc->mp->m_sb.sb_rextsize);
> -	if (do_div(endext, sc->mp->m_sb.sb_rextsize))
> -		endext++;
> -	extcount = endext - startext;
> +	do_div(endext, sc->mp->m_sb.sb_rextsize);
> +	extcount = endext - startext + 1;
>  	xfs_ilock(sc->mp->m_rbmip, XFS_ILOCK_SHARED | XFS_ILOCK_RTBITMAP);
>  	error = xfs_rtalloc_extent_is_free(sc->mp, sc->tp, startext, extcount,
>  			&is_free);

Re: [PATCH v2 3/5] xfs: fix off-by-one error in rtbitmap cross-reference

[Darrick J. Wong]

On Tue, Jan 15, 2019 at 05:49:54AM -0500, Brian Foster wrote:
> On Mon, Jan 14, 2019 at 12:32:01PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Fix an off-by-one error in the realtime bitmap "is used" cross-reference
> > helper function if the realtime extent size is a single block.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks for reviewing this series!

--D

> >  fs/xfs/scrub/rtbitmap.c |    5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
> > index 665d4bbb17cc..dbe115b075f7 100644
> > --- a/fs/xfs/scrub/rtbitmap.c
> > +++ b/fs/xfs/scrub/rtbitmap.c
> > @@ -141,9 +141,8 @@ xchk_xref_is_used_rt_space(
> >  	startext = fsbno;
> >  	endext = fsbno + len - 1;
> >  	do_div(startext, sc->mp->m_sb.sb_rextsize);
> > -	if (do_div(endext, sc->mp->m_sb.sb_rextsize))
> > -		endext++;
> > -	extcount = endext - startext;
> > +	do_div(endext, sc->mp->m_sb.sb_rextsize);
> > +	extcount = endext - startext + 1;
> >  	xfs_ilock(sc->mp->m_rbmip, XFS_ILOCK_SHARED | XFS_ILOCK_RTBITMAP);
> >  	error = xfs_rtalloc_extent_is_free(sc->mp, sc->tp, startext, extcount,
> >  			&is_free);

[PATCH 4/5] xfs: check directory name validity

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check directory entry names for invalid characters.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dir2.c |   17 +++++++++++++++++
 fs/xfs/libxfs/xfs_dir2.h |    1 +
 fs/xfs/scrub/dir.c       |    6 ++++++
 3 files changed, 24 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index 229152cd1a24..156ce95c9c45 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -703,3 +703,20 @@ xfs_dir2_shrink_inode(
 	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
 	return 0;
 }
+
+/* Returns true if the directory entry name is valid. */
+bool
+xfs_dir2_namecheck(
+	const void	*name,
+	size_t		length)
+{
+	/*
+	 * MAXNAMELEN includes the trailing null, but (name/length) leave it
+	 * out, so use >= for the length check.
+	 */
+	if (length >= MAXNAMELEN)
+		return false;
+
+	/* There shouldn't be any slashes or nulls here */
+	return !memchr(name, '/', length) && !memchr(name, 0, length);
+}
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index c3e3f6b813d8..f54244779492 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -326,5 +326,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
 unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
 void *xfs_dir3_data_endp(struct xfs_da_geometry *geo,
 		struct xfs_dir2_data_hdr *hdr);
+bool xfs_dir2_namecheck(const void *name, size_t length);
 
 #endif	/* __XFS_DIR2_H__ */
diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index cd3e4d768a18..a38a22785a1a 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -129,6 +129,12 @@ xchk_dir_actor(
 		goto out;
 	}
 
+	/* Does this name make sense? */
+	if (!xfs_dir2_namecheck(name, namelen)) {
+		xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
+		goto out;
+	}
+
 	if (!strncmp(".", name, namelen)) {
 		/* If this is "." then check that the inum matches the dir. */
 		if (xfs_sb_version_hasftype(&mp->m_sb) && type != DT_DIR)

Re: [PATCH 4/5] xfs: check directory name validity

[Brian Foster]

On Tue, Jan 08, 2019 at 12:36:29PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check directory entry names for invalid characters.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_dir2.c |   17 +++++++++++++++++
>  fs/xfs/libxfs/xfs_dir2.h |    1 +
>  fs/xfs/scrub/dir.c       |    6 ++++++
>  3 files changed, 24 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 229152cd1a24..156ce95c9c45 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -703,3 +703,20 @@ xfs_dir2_shrink_inode(
>  	xfs_trans_log_inode(tp, dp, XFS_ILOG_CORE);
>  	return 0;
>  }
> +
> +/* Returns true if the directory entry name is valid. */
> +bool
> +xfs_dir2_namecheck(
> +	const void	*name,
> +	size_t		length)
> +{
> +	/*
> +	 * MAXNAMELEN includes the trailing null, but (name/length) leave it
> +	 * out, so use >= for the length check.
> +	 */
> +	if (length >= MAXNAMELEN)
> +		return false;
> +
> +	/* There shouldn't be any slashes or nulls here */
> +	return !memchr(name, '/', length) && !memchr(name, 0, length);
> +}
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index c3e3f6b813d8..f54244779492 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -326,5 +326,6 @@ xfs_dir2_leaf_tail_p(struct xfs_da_geometry *geo, struct xfs_dir2_leaf *lp)
>  unsigned char xfs_dir3_get_dtype(struct xfs_mount *mp, uint8_t filetype);
>  void *xfs_dir3_data_endp(struct xfs_da_geometry *geo,
>  		struct xfs_dir2_data_hdr *hdr);
> +bool xfs_dir2_namecheck(const void *name, size_t length);
>  
>  #endif	/* __XFS_DIR2_H__ */
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index cd3e4d768a18..a38a22785a1a 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -129,6 +129,12 @@ xchk_dir_actor(
>  		goto out;
>  	}
>  
> +	/* Does this name make sense? */
> +	if (!xfs_dir2_namecheck(name, namelen)) {
> +		xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> +		goto out;
> +	}
> +
>  	if (!strncmp(".", name, namelen)) {
>  		/* If this is "." then check that the inum matches the dir. */
>  		if (xfs_sb_version_hasftype(&mp->m_sb) && type != DT_DIR)
> 

[PATCH 5/5] xfs: check attribute name validity

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Check extended attribute entry names for invalid characters.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c |   17 +++++++++++++++++
 fs/xfs/libxfs/xfs_attr.h |    2 +-
 fs/xfs/scrub/attr.c      |    6 ++++++
 3 files changed, 24 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 844ed87b1900..2dd9ee2a2e08 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1336,3 +1336,20 @@ xfs_attr_node_get(xfs_da_args_t *args)
 	xfs_da_state_free(state);
 	return retval;
 }
+
+/* Returns true if the attribute entry name is valid. */
+bool
+xfs_attr_namecheck(
+	const void	*name,
+	size_t		length)
+{
+	/*
+	 * MAXNAMELEN includes the trailing null, but (name/length) leave it
+	 * out, so use >= for the length check.
+	 */
+	if (length >= MAXNAMELEN)
+		return false;
+
+	/* There shouldn't be any nulls here */
+	return !memchr(name, 0, length);
+}
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index bdf52a333f3f..2297d8467666 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -145,6 +145,6 @@ int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
 int xfs_attr_remove_args(struct xfs_da_args *args);
 int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
 		  int flags, struct attrlist_cursor_kern *cursor);
-
+bool xfs_attr_namecheck(const void *name, size_t length);
 
 #endif	/* __XFS_ATTR_H__ */
diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index 9960bc5b5d76..dce74ec57038 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -93,6 +93,12 @@ xchk_xattr_listent(
 		return;
 	}
 
+	/* Does this name make sense? */
+	if (!xfs_attr_namecheck(name, namelen)) {
+		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
+		return;
+	}
+
 	args.flags = ATTR_KERNOTIME;
 	if (flags & XFS_ATTR_ROOT)
 		args.flags |= ATTR_ROOT;

Re: [PATCH 5/5] xfs: check attribute name validity

[Brian Foster]

On Tue, Jan 08, 2019 at 12:36:35PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check extended attribute entry names for invalid characters.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_attr.c |   17 +++++++++++++++++
>  fs/xfs/libxfs/xfs_attr.h |    2 +-
>  fs/xfs/scrub/attr.c      |    6 ++++++
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 844ed87b1900..2dd9ee2a2e08 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1336,3 +1336,20 @@ xfs_attr_node_get(xfs_da_args_t *args)
>  	xfs_da_state_free(state);
>  	return retval;
>  }
> +
> +/* Returns true if the attribute entry name is valid. */
> +bool
> +xfs_attr_namecheck(
> +	const void	*name,
> +	size_t		length)
> +{
> +	/*
> +	 * MAXNAMELEN includes the trailing null, but (name/length) leave it
> +	 * out, so use >= for the length check.
> +	 */
> +	if (length >= MAXNAMELEN)
> +		return false;
> +
> +	/* There shouldn't be any nulls here */
> +	return !memchr(name, 0, length);
> +}
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index bdf52a333f3f..2297d8467666 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -145,6 +145,6 @@ int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
>  int xfs_attr_remove_args(struct xfs_da_args *args);
>  int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
>  		  int flags, struct attrlist_cursor_kern *cursor);
> -
> +bool xfs_attr_namecheck(const void *name, size_t length);
>  
>  #endif	/* __XFS_ATTR_H__ */
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index 9960bc5b5d76..dce74ec57038 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -93,6 +93,12 @@ xchk_xattr_listent(
>  		return;
>  	}
>  
> +	/* Does this name make sense? */
> +	if (!xfs_attr_namecheck(name, namelen)) {
> +		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
> +		return;
> +	}
> +
>  	args.flags = ATTR_KERNOTIME;
>  	if (flags & XFS_ATTR_ROOT)
>  		args.flags |= ATTR_ROOT;
> 

[PATCH 6/5] libxfs(progs): fix attr include mess

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Remove all the userspace xfs_attr shim cruft so that we have one
definition of ATTR_* macros so that we can actually use xfs_attr.c
routines and include xfs_attr.h without problems.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/libxfs.h         |   10 +---------
 libxfs/libxfs_api_defs.h |    5 +++++
 libxfs/libxfs_priv.h     |    8 --------
 libxfs/xfs_attr.c        |    1 +
 libxfs/xfs_attr_leaf.c   |    1 +
 5 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/include/libxfs.h b/include/libxfs.h
index 2bdef706..32dbb1c6 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -214,14 +214,6 @@ libxfs_bmbt_disk_get_all(
 int libxfs_rtfree_extent(struct xfs_trans *, xfs_rtblock_t, xfs_extlen_t);
 bool libxfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
 
-/* XXX: need parts of xfs_attr.h in userspace */
-#define LIBXFS_ATTR_ROOT	0x0002	/* use attrs in root namespace */
-#define LIBXFS_ATTR_SECURE	0x0008	/* use attrs in security namespace */
-#define LIBXFS_ATTR_CREATE	0x0010	/* create, but fail if attr exists */
-#define LIBXFS_ATTR_REPLACE	0x0020	/* set, but fail if attr not exists */
-
-int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
-int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
-		 unsigned char *value, int valuelen, int flags);
+#include "xfs_attr.h"
 
 #endif	/* __LIBXFS_H__ */
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index e10d78cd..2d5ff88f 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -150,4 +150,9 @@
 #define xfs_fs_geometry			libxfs_fs_geometry
 #define xfs_init_local_fork		libxfs_init_local_fork
 
+#define LIBXFS_ATTR_ROOT		ATTR_ROOT
+#define LIBXFS_ATTR_SECURE		ATTR_SECURE
+#define LIBXFS_ATTR_CREATE		ATTR_CREATE
+#define LIBXFS_ATTR_REPLACE		ATTR_REPLACE
+
 #endif /* __LIBXFS_API_DEFS_H__ */
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index b45d07eb..59cd3333 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -104,14 +104,6 @@ extern char    *progname;
  */
 #define PTR_FMT "%p"
 
-/* XXX: need to push these out to make LIBXFS_ATTR defines */
-#define ATTR_ROOT			0x0002
-#define ATTR_SECURE			0x0008
-#define ATTR_CREATE			0x0010
-#define ATTR_REPLACE			0x0020
-#define ATTR_KERNOTIME			0
-#define ATTR_KERNOVAL			0
-
 #define XFS_IGET_CREATE			0x1
 #define XFS_IGET_UNTRUSTED		0x2
 
diff --git a/libxfs/xfs_attr.c b/libxfs/xfs_attr.c
index b8838302..170e64cf 100644
--- a/libxfs/xfs_attr.c
+++ b/libxfs/xfs_attr.c
@@ -20,6 +20,7 @@
 #include "xfs_trans.h"
 #include "xfs_bmap.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_attr.h"
 #include "xfs_attr_leaf.h"
 #include "xfs_attr_remote.h"
 #include "xfs_trans_space.h"
diff --git a/libxfs/xfs_attr_leaf.c b/libxfs/xfs_attr_leaf.c
index 5683d294..48f4b28d 100644
--- a/libxfs/xfs_attr_leaf.c
+++ b/libxfs/xfs_attr_leaf.c
@@ -21,6 +21,7 @@
 #include "xfs_bmap.h"
 #include "xfs_attr_sf.h"
 #include "xfs_attr_remote.h"
+#include "xfs_attr.h"
 #include "xfs_attr_leaf.h"
 #include "xfs_trace.h"
 #include "xfs_cksum.h"

[PATCH 7/5] xfs_repair: refactor namecheck functions

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Now that we have name check functions in libxfs, use them instead of our
custom versions.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    2 ++
 repair/attr_repair.c     |   32 +++++++++++++-------------------
 repair/da_util.c         |   26 --------------------------
 repair/da_util.h         |    6 ------
 repair/dir2.c            |   12 ++----------
 5 files changed, 17 insertions(+), 61 deletions(-)

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 2d5ff88f..c8082a60 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -149,6 +149,8 @@
 #define xfs_default_ifork_ops		libxfs_default_ifork_ops
 #define xfs_fs_geometry			libxfs_fs_geometry
 #define xfs_init_local_fork		libxfs_init_local_fork
+#define xfs_dir2_namecheck		libxfs_dir2_namecheck
+#define xfs_attr_namecheck		libxfs_attr_namecheck
 
 #define LIBXFS_ATTR_ROOT		ATTR_ROOT
 #define LIBXFS_ATTR_SECURE		ATTR_SECURE
diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 5ad81c09..9a44f610 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -122,14 +122,6 @@ set_da_freemap(xfs_mount_t *mp, da_freemap_t *map, int start, int stop)
  * fork being emptied and put in shortform format.
  */
 
-static int
-attr_namecheck(
-	uint8_t	*name,
-	int	length)
-{
-	return namecheck((char *)name, length, false);
-}
-
 /*
  * This routine just checks what security needs are for attribute values
  * only called when root flag is set, otherwise these names could exist in
@@ -301,8 +293,8 @@ process_shortform_attr(
 		}
 
 		/* namecheck checks for null chars in attr names. */
-		if (attr_namecheck(currententry->nameval,
-						currententry->namelen)) {
+		if (!libxfs_attr_namecheck(currententry->nameval,
+					   currententry->namelen)) {
 			do_warn(
 	_("entry contains illegal character in shortform attribute name\n"));
 			junkit = 1;
@@ -464,8 +456,9 @@ process_leaf_attr_local(
 	xfs_attr_leaf_name_local_t *local;
 
 	local = xfs_attr3_leaf_name_local(leaf, i);
-	if (local->namelen == 0 || attr_namecheck(local->nameval,
-							local->namelen)) {
+	if (local->namelen == 0 ||
+	    !libxfs_attr_namecheck(local->nameval,
+				   local->namelen)) {
 		do_warn(
 	_("attribute entry %d in attr block %u, inode %" PRIu64 " has bad name (namelen = %d)\n"),
 			i, da_bno, ino, local->namelen);
@@ -519,13 +512,14 @@ process_leaf_attr_remote(
 
 	remotep = xfs_attr3_leaf_name_remote(leaf, i);
 
-	if (remotep->namelen == 0 || attr_namecheck(remotep->name,
-						remotep->namelen) ||
-			be32_to_cpu(entry->hashval) !=
-				libxfs_da_hashname((unsigned char *)&remotep->name[0],
-						remotep->namelen) ||
-			be32_to_cpu(entry->hashval) < last_hashval ||
-			be32_to_cpu(remotep->valueblk) == 0) {
+	if (remotep->namelen == 0 ||
+	    !libxfs_attr_namecheck(remotep->name,
+				   remotep->namelen) ||
+	    be32_to_cpu(entry->hashval) !=
+			libxfs_da_hashname((unsigned char *)&remotep->name[0],
+					   remotep->namelen) ||
+	    be32_to_cpu(entry->hashval) < last_hashval ||
+	    be32_to_cpu(remotep->valueblk) == 0) {
 		do_warn(
 	_("inconsistent remote attribute entry %d in attr block %u, ino %" PRIu64 "\n"), i, da_bno, ino);
 		return -1;
diff --git a/repair/da_util.c b/repair/da_util.c
index 7ad8e492..8c818ea1 100644
--- a/repair/da_util.c
+++ b/repair/da_util.c
@@ -12,32 +12,6 @@
 #include "bmap.h"
 #include "da_util.h"
 
-/*
- * takes a name and length (name need not be null-terminated) and whether
- * we are checking a dir (vs an attr), and returns 1 if the direntry contains
- * a '/', or anything contains a \0, returns 0 otherwise
- */
-int
-namecheck(
-	char	*name,
-	int	length,
-	bool	isadir)
-{
-	char	*c;
-	int	i;
-
-	ASSERT(length < MAXNAMELEN);
-
-	for (c = name, i = 0; i < length; i++, c++) {
-		if (isadir && *c == '/')
-			return 1;
-		if (*c == '\0')
-			return 1;
-	}
-
-	return 0;
-}
-
 /*
  * the cursor gets passed up and down the da btree processing
  * routines.  The interior block processing routines use the
diff --git a/repair/da_util.h b/repair/da_util.h
index 041dff74..90fec00c 100644
--- a/repair/da_util.h
+++ b/repair/da_util.h
@@ -24,12 +24,6 @@ typedef struct da_bt_cursor {
 	struct blkmap		*blkmap;
 } da_bt_cursor_t;
 
-int
-namecheck(
-	char		*name,
-	int		length,
-	bool		isadir);
-
 struct xfs_buf *
 da_read_buf(
 	xfs_mount_t	*mp,
diff --git a/repair/dir2.c b/repair/dir2.c
index 094ecb3d..4ac0084e 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -44,14 +44,6 @@ _("malloc failed (%zu bytes) dir2_add_badlist:ino %" PRIu64 "\n"),
 	l->ino = ino;
 }
 
-static int
-dir_namecheck(
-	uint8_t	*name,
-	int	length)
-{
-	return namecheck((char *)name, length, true);
-}
-
 int
 dir2_is_badino(
 	xfs_ino_t	ino)
@@ -318,7 +310,7 @@ _("entry #%d %s in shortform dir %" PRIu64),
 		 * the length value is stored in a byte
 		 * so it can't be too big, it can only wrap
 		 */
-		if (dir_namecheck(sfep->name, namelen)) {
+		if (!libxfs_dir2_namecheck(sfep->name, namelen)) {
 			/*
 			 * junk entry
 			 */
@@ -789,7 +781,7 @@ _("\twould clear inode number in entry at offset %" PRIdPTR "...\n"),
 		 * during phase 4.
 		 */
 		junkit = dep->name[0] == '/';
-		nm_illegal = dir_namecheck(dep->name, dep->namelen);
+		nm_illegal = !libxfs_dir2_namecheck(dep->name, dep->namelen);
 		if (ino_discovery && nm_illegal) {
 			do_warn(
 _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 " has illegal name \"%*.*s\": "),