All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets"
@ 2020-11-03  2:33 Gao Xiang
  2020-11-03  2:33 ` [PATCH v2 2/2] xfsdump: intercept bind mount targets Gao Xiang
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Gao Xiang @ 2020-11-03  2:33 UTC (permalink / raw)
  To: linux-xfs; +Cc: Darrick J. Wong, Gao Xiang, Eric Sandeen

Bind mount mntpnts will be forbided in the next commits
instead since it's not the real rootdir.

This cannot be reverted cleanly due to several cleanup
patches, but the logic is reverted equivalently.

This reverts commit 25195ebf107dc81b1b7cea1476764950e1d6cc9d.

Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 dump/content.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/dump/content.c b/dump/content.c
index 30232d4..c11d9b4 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -1382,17 +1382,10 @@ baseuuidbypass:
 	}
 
 	/* figure out the ino for the root directory of the fs
-	 * and get its struct xfs_bstat for inomap_build().  This could
-	 * be a bind mount; don't ask for the mount point inode,
-	 * find the actual lowest inode number in the filesystem.
+	 * and get its xfs_bstat_t for inomap_build()
 	 */
 	{
 		stat64_t rootstat;
-		xfs_ino_t lastino = 0;
-		int ocount = 0;
-		struct xfs_fsop_bulkreq bulkreq;
-
-		/* Get the inode of the mount point */
 		rval = fstat64(sc_fsfd, &rootstat);
 		if (rval) {
 			mlog(MLOG_NORMAL, _(
@@ -1404,21 +1397,11 @@ baseuuidbypass:
 			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
 		assert(sc_rootxfsstatp);
 
-		/* Get the first valid (i.e. root) inode in this fs */
-		bulkreq.lastip = (__u64 *)&lastino;
-		bulkreq.icount = 1;
-		bulkreq.ubuffer = sc_rootxfsstatp;
-		bulkreq.ocount = &ocount;
-		if (ioctl(sc_fsfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) {
+		if (bigstat_one(sc_fsfd, rootstat.st_ino, sc_rootxfsstatp) < 0) {
 			mlog(MLOG_ERROR,
 			      _("failed to get bulkstat information for root inode\n"));
 			return BOOL_FALSE;
 		}
-
-		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
-			mlog (MLOG_NORMAL | MLOG_NOTE,
-			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
-			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
 	}
 
 	/* alloc a file system handle, to be used with the jdm_open()
-- 
2.18.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] xfsdump: intercept bind mount targets
  2020-11-03  2:33 [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets" Gao Xiang
@ 2020-11-03  2:33 ` Gao Xiang
  2020-11-03 15:03   ` Eric Sandeen
  2020-11-03 15:33   ` [PATCH v3 " Gao Xiang
  2021-06-03 20:29 ` [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets" Eric Sandeen
  2021-09-27 15:05 ` Bill O'Donnell
  2 siblings, 2 replies; 9+ messages in thread
From: Gao Xiang @ 2020-11-03  2:33 UTC (permalink / raw)
  To: linux-xfs; +Cc: Darrick J. Wong, Gao Xiang, Eric Sandeen

It's a bit strange pointing at some non-root bind mount target and
then actually dumping from the actual root dir instead.

Therefore, instead of searching for the root dir of the filesystem,
just intercept all bind mount targets by checking whose ino # of
".." is itself with getdents.

Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v1:
 just intercept bind mount targets suggested by Eric on IRC instead,
 and no need to use XFS_BULK_IREQ_SPECIAL_ROOT since the xfs header
 files and the kernel on my laptop don't support it, plus
 xfsdump/xfsrestore are not part of xfsprogs, so a bit hard to
 sync and make full use of that elegantly.

 dump/content.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/dump/content.c b/dump/content.c
index c11d9b4..7a55b92 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -511,6 +511,61 @@ static bool_t create_inv_session(
 		ix_t subtreecnt,
 		size_t strmix);
 
+static bool_t
+check_rootdir(int fd,
+	      xfs_ino_t ino)
+{
+	struct dirent	*gdp;
+	size_t		gdsz;
+
+	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
+	if (gdsz < GETDENTSBUF_SZ_MIN)
+		gdsz = GETDENTSBUF_SZ_MIN;
+	gdp = (struct dirent *)calloc(1, gdsz);
+	assert(gdp);
+
+	while (1) {
+		struct dirent *p;
+		int nread;
+
+		nread = getdents_wrap(fd, (char *)gdp, gdsz);
+		/*
+		 * negative count indicates something very bad happened;
+		 * try to gracefully end this dir.
+		 */
+		if (nread < 0) {
+			mlog(MLOG_NORMAL | MLOG_WARNING,
+_("unable to read dirents for directory ino %llu: %s\n"),
+			      ino, strerror(errno));
+			/* !!! curtis looked at this, and pointed out that
+			 * we could take some recovery action here. if the
+			 * errno is appropriate, lseek64 to the value of
+			 * doff field of the last dirent successfully
+			 * obtained, and contiue the loop.
+			 */
+			nread = 0; /* pretend we are done */
+		}
+
+		/* no more directory entries: break; */
+		if (!nread)
+			break;
+
+		for (p = gdp; nread > 0;
+		     nread -= (int)p->d_reclen,
+		     assert(nread >= 0),
+		     p = (struct dirent *)((char *)p + p->d_reclen)) {
+			if (!strcmp(p->d_name, "..") && p->d_ino == ino) {
+				mlog(MLOG_DEBUG, "FOUND: name %s d_ino %llu\n",
+				     p->d_name, ino);
+				free(gdp);
+				return BOOL_TRUE;
+			}
+		}
+	}
+	free(gdp);
+	return BOOL_FALSE;
+}
+
 bool_t
 content_init(int argc,
 	      char *argv[],
@@ -1393,6 +1448,13 @@ baseuuidbypass:
 			      mntpnt);
 			return BOOL_FALSE;
 		}
+
+		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
+			mlog(MLOG_ERROR,
+"oops, seems to be a bind mount, please use the actual mountpoint instead\n");
+			return BOOL_FALSE;
+		}
+
 		sc_rootxfsstatp =
 			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
 		assert(sc_rootxfsstatp);
-- 
2.18.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] xfsdump: intercept bind mount targets
  2020-11-03  2:33 ` [PATCH v2 2/2] xfsdump: intercept bind mount targets Gao Xiang
@ 2020-11-03 15:03   ` Eric Sandeen
  2020-11-03 15:18     ` Gao Xiang
  2020-11-03 15:33   ` [PATCH v3 " Gao Xiang
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2020-11-03 15:03 UTC (permalink / raw)
  To: Gao Xiang, linux-xfs; +Cc: Darrick J. Wong, Eric Sandeen

On 11/2/20 8:33 PM, Gao Xiang wrote:
> It's a bit strange pointing at some non-root bind mount target and
> then actually dumping from the actual root dir instead.
> 
> Therefore, instead of searching for the root dir of the filesystem,
> just intercept all bind mount targets by checking whose ino # of
> ".." is itself with getdents.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v1:
>  just intercept bind mount targets suggested by Eric on IRC instead,
>  and no need to use XFS_BULK_IREQ_SPECIAL_ROOT since the xfs header
>  files and the kernel on my laptop don't support it, plus
>  xfsdump/xfsrestore are not part of xfsprogs, so a bit hard to
>  sync and make full use of that elegantly.

Thank you for doing this - it seems to be the least intrusive method to
finally fix this problem, I really appreciate you taking the work.

> 
>  dump/content.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/dump/content.c b/dump/content.c
> index c11d9b4..7a55b92 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -511,6 +511,61 @@ static bool_t create_inv_session(
>  		ix_t subtreecnt,
>  		size_t strmix);
>  
> +static bool_t
> +check_rootdir(int fd,
> +	      xfs_ino_t ino)
> +{
> +	struct dirent	*gdp;
> +	size_t		gdsz;
> +
> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
> +	if (gdsz < GETDENTSBUF_SZ_MIN)
> +		gdsz = GETDENTSBUF_SZ_MIN;
> +	gdp = (struct dirent *)calloc(1, gdsz);
> +	assert(gdp);
> +
> +	while (1) {
> +		struct dirent *p;
> +		int nread;
> +
> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> +		/*
> +		 * negative count indicates something very bad happened;
> +		 * try to gracefully end this dir.
> +		 */
> +		if (nread < 0) {
> +			mlog(MLOG_NORMAL | MLOG_WARNING,
> +_("unable to read dirents for directory ino %llu: %s\n"),
> +			      ino, strerror(errno));
> +			/* !!! curtis looked at this, and pointed out that

Nobody knows who curtis is, I think we can drop this comment now ;)
If we can't read the directory I think it's fine to simply error out here.

> +			 * we could take some recovery action here. if the
> +			 * errno is appropriate, lseek64 to the value of
> +			 * doff field of the last dirent successfully
> +			 * obtained, and contiue the loop.
> +			 */
> +			nread = 0; /* pretend we are done */
> +		}
> +
> +		/* no more directory entries: break; */
> +		if (!nread)
> +			break;
> +
> +		for (p = gdp; nread > 0;
> +		     nread -= (int)p->d_reclen,
> +		     assert(nread >= 0),
> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> +			if (!strcmp(p->d_name, "..") && p->d_ino == ino) {
> +				mlog(MLOG_DEBUG, "FOUND: name %s d_ino %llu\n",
> +				     p->d_name, ino);
> +				free(gdp);
> +				return BOOL_TRUE;
> +			}

I think we can stop as soon as we have found ".." yes?  No need to continue
iterating the directory, either ".." is what we wanted, or it's not, but either
way we are done when we have checked it.  On the off chance that we have
a very large root dir, stopping early might be good.

> +		}
> +	}
> +	free(gdp);
> +	return BOOL_FALSE;
> +}
> +
>  bool_t
>  content_init(int argc,
>  	      char *argv[],
> @@ -1393,6 +1448,13 @@ baseuuidbypass:
>  			      mntpnt);
>  			return BOOL_FALSE;
>  		}
> +
> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> +			mlog(MLOG_ERROR,
> +"oops, seems to be a bind mount, please use the actual mountpoint instead\n");

Could there be any other reason for this failure?  Maybe something like:

			mlog(MLOG_ERROR,
_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
				mntpnt);

or similar?

in any case I think it needs the i18n _("...") treatment.

Thanks!

-Eric

> +			return BOOL_FALSE;
> +		}
> +
>  		sc_rootxfsstatp =
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] xfsdump: intercept bind mount targets
  2020-11-03 15:03   ` Eric Sandeen
@ 2020-11-03 15:18     ` Gao Xiang
  0 siblings, 0 replies; 9+ messages in thread
From: Gao Xiang @ 2020-11-03 15:18 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs, Darrick J. Wong, Eric Sandeen


Hi Eric,

On Tue, Nov 03, 2020 at 09:03:48AM -0600, Eric Sandeen wrote:
> On 11/2/20 8:33 PM, Gao Xiang wrote:

...

> > +
> > +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> > +		/*
> > +		 * negative count indicates something very bad happened;
> > +		 * try to gracefully end this dir.
> > +		 */
> > +		if (nread < 0) {
> > +			mlog(MLOG_NORMAL | MLOG_WARNING,
> > +_("unable to read dirents for directory ino %llu: %s\n"),
> > +			      ino, strerror(errno));
> > +			/* !!! curtis looked at this, and pointed out that
> 
> Nobody knows who curtis is, I think we can drop this comment now ;)
> If we can't read the directory I think it's fine to simply error out here.

This was copied from dump_dir(), ok, I will error out this.

> 
> > +			 * we could take some recovery action here. if the
> > +			 * errno is appropriate, lseek64 to the value of
> > +			 * doff field of the last dirent successfully
> > +			 * obtained, and contiue the loop.
> > +			 */
> > +			nread = 0; /* pretend we are done */
> > +		}
> > +
> > +		/* no more directory entries: break; */
> > +		if (!nread)
> > +			break;
> > +
> > +		for (p = gdp; nread > 0;
> > +		     nread -= (int)p->d_reclen,
> > +		     assert(nread >= 0),
> > +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> > +			if (!strcmp(p->d_name, "..") && p->d_ino == ino) {
> > +				mlog(MLOG_DEBUG, "FOUND: name %s d_ino %llu\n",
> > +				     p->d_name, ino);
> > +				free(gdp);
> > +				return BOOL_TRUE;
> > +			}
> 
> I think we can stop as soon as we have found ".." yes?  No need to continue
> iterating the directory, either ".." is what we wanted, or it's not, but either
> way we are done when we have checked it.  On the off chance that we have
> a very large root dir, stopping early might be good.

Yes, that is correct.

> 
> > +		}
> > +	}
> > +	free(gdp);
> > +	return BOOL_FALSE;
> > +}
> > +
> >  bool_t
> >  content_init(int argc,
> >  	      char *argv[],
> > @@ -1393,6 +1448,13 @@ baseuuidbypass:
> >  			      mntpnt);
> >  			return BOOL_FALSE;
> >  		}
> > +
> > +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> > +			mlog(MLOG_ERROR,
> > +"oops, seems to be a bind mount, please use the actual mountpoint instead\n");
> 
> Could there be any other reason for this failure?  Maybe something like:
> 
> 			mlog(MLOG_ERROR,
> _("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
> 				mntpnt);
> 
> or similar?
> 
> in any case I think it needs the i18n _("...") treatment.

Ok, will quickly send the fixed version about this!


Thanks for your review!


Thanks,
Gao Xiang

> 
> Thanks!
> 
> -Eric
> 
> > +			return BOOL_FALSE;
> > +		}
> > +
> >  		sc_rootxfsstatp =
> >  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
> >  		assert(sc_rootxfsstatp);
> > 
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 2/2] xfsdump: intercept bind mount targets
  2020-11-03  2:33 ` [PATCH v2 2/2] xfsdump: intercept bind mount targets Gao Xiang
  2020-11-03 15:03   ` Eric Sandeen
@ 2020-11-03 15:33   ` Gao Xiang
  2021-06-03 20:53     ` Eric Sandeen
  2021-09-27 15:07     ` Bill O'Donnell
  1 sibling, 2 replies; 9+ messages in thread
From: Gao Xiang @ 2020-11-03 15:33 UTC (permalink / raw)
  To: linux-xfs; +Cc: Darrick J. Wong, Gao Xiang, Eric Sandeen

It's a bit strange pointing at some non-root bind mount target and
then actually dumping from the actual root dir instead.

Therefore, instead of searching for the root dir of the filesystem,
just intercept all bind mount targets by checking whose ino # of
".." is itself with getdents.

Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
Cc: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v2 (Eric):
 - error out the case where the directory cannot be read;
 - In any case, stop as soon as we have found "..";
 - update the mountpoint error message and use i18n instead;

 dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/dump/content.c b/dump/content.c
index c11d9b4..c248e74 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -511,6 +511,55 @@ static bool_t create_inv_session(
 		ix_t subtreecnt,
 		size_t strmix);
 
+static bool_t
+check_rootdir(int fd,
+	      xfs_ino_t ino)
+{
+	struct dirent	*gdp;
+	size_t		gdsz;
+	bool_t		found = BOOL_FALSE;
+
+	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
+	if (gdsz < GETDENTSBUF_SZ_MIN)
+		gdsz = GETDENTSBUF_SZ_MIN;
+	gdp = (struct dirent *)calloc(1, gdsz);
+	assert(gdp);
+
+	while (1) {
+		struct dirent *p;
+		int nread;
+
+		nread = getdents_wrap(fd, (char *)gdp, gdsz);
+		/*
+		 * negative count indicates something very bad happened;
+		 * try to gracefully end this dir.
+		 */
+		if (nread < 0) {
+			mlog(MLOG_NORMAL | MLOG_WARNING,
+_("unable to read dirents for directory ino %llu: %s\n"),
+			      ino, strerror(errno));
+			break;
+		}
+
+		/* no more directory entries: break; */
+		if (!nread)
+			break;
+
+		for (p = gdp; nread > 0;
+		     nread -= (int)p->d_reclen,
+		     assert(nread >= 0),
+		     p = (struct dirent *)((char *)p + p->d_reclen)) {
+			if (!strcmp(p->d_name, "..")) {
+				if (p->d_ino == ino)
+					found = BOOL_TRUE;
+				break;
+			}
+		}
+	}
+	free(gdp);
+	return found;
+}
+
 bool_t
 content_init(int argc,
 	      char *argv[],
@@ -1393,6 +1442,14 @@ baseuuidbypass:
 			      mntpnt);
 			return BOOL_FALSE;
 		}
+
+		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
+			mlog(MLOG_ERROR,
+_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
+			     mntpnt);
+			return BOOL_FALSE;
+		}
+
 		sc_rootxfsstatp =
 			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
 		assert(sc_rootxfsstatp);
-- 
2.18.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets"
  2020-11-03  2:33 [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets" Gao Xiang
  2020-11-03  2:33 ` [PATCH v2 2/2] xfsdump: intercept bind mount targets Gao Xiang
@ 2021-06-03 20:29 ` Eric Sandeen
  2021-09-27 15:05 ` Bill O'Donnell
  2 siblings, 0 replies; 9+ messages in thread
From: Eric Sandeen @ 2021-06-03 20:29 UTC (permalink / raw)
  To: Gao Xiang, linux-xfs; +Cc: Darrick J. Wong, Eric Sandeen

On 11/2/20 8:33 PM, Gao Xiang wrote:
> Bind mount mntpnts will be forbided in the next commits
> instead since it's not the real rootdir.
> 
> This cannot be reverted cleanly due to several cleanup
> patches, but the logic is reverted equivalently.
> 
> This reverts commit 25195ebf107dc81b1b7cea1476764950e1d6cc9d.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Sorry this is so ridiculously late:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  dump/content.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/dump/content.c b/dump/content.c
> index 30232d4..c11d9b4 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -1382,17 +1382,10 @@ baseuuidbypass:
>  	}
>  
>  	/* figure out the ino for the root directory of the fs
> -	 * and get its struct xfs_bstat for inomap_build().  This could
> -	 * be a bind mount; don't ask for the mount point inode,
> -	 * find the actual lowest inode number in the filesystem.
> +	 * and get its xfs_bstat_t for inomap_build()
>  	 */
>  	{
>  		stat64_t rootstat;
> -		xfs_ino_t lastino = 0;
> -		int ocount = 0;
> -		struct xfs_fsop_bulkreq bulkreq;
> -
> -		/* Get the inode of the mount point */
>  		rval = fstat64(sc_fsfd, &rootstat);
>  		if (rval) {
>  			mlog(MLOG_NORMAL, _(
> @@ -1404,21 +1397,11 @@ baseuuidbypass:
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
>  
> -		/* Get the first valid (i.e. root) inode in this fs */
> -		bulkreq.lastip = (__u64 *)&lastino;
> -		bulkreq.icount = 1;
> -		bulkreq.ubuffer = sc_rootxfsstatp;
> -		bulkreq.ocount = &ocount;
> -		if (ioctl(sc_fsfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) {
> +		if (bigstat_one(sc_fsfd, rootstat.st_ino, sc_rootxfsstatp) < 0) {
>  			mlog(MLOG_ERROR,
>  			      _("failed to get bulkstat information for root inode\n"));
>  			return BOOL_FALSE;
>  		}
> -
> -		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
> -			mlog (MLOG_NORMAL | MLOG_NOTE,
> -			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
> -			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
>  	}
>  
>  	/* alloc a file system handle, to be used with the jdm_open()
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/2] xfsdump: intercept bind mount targets
  2020-11-03 15:33   ` [PATCH v3 " Gao Xiang
@ 2021-06-03 20:53     ` Eric Sandeen
  2021-09-27 15:07     ` Bill O'Donnell
  1 sibling, 0 replies; 9+ messages in thread
From: Eric Sandeen @ 2021-06-03 20:53 UTC (permalink / raw)
  To: Gao Xiang, linux-xfs; +Cc: Darrick J. Wong, Eric Sandeen

On 11/3/20 9:33 AM, Gao Xiang wrote:
> It's a bit strange pointing at some non-root bind mount target and
> then actually dumping from the actual root dir instead.
> 
> Therefore, instead of searching for the root dir of the filesystem,
> just intercept all bind mount targets by checking whose ino # of
> ".." is itself with getdents.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v2 (Eric):
>  - error out the case where the directory cannot be read;
>  - In any case, stop as soon as we have found "..";
>  - update the mountpoint error message and use i18n instead;

This looks fine, thank you. I wish I'd done it properly the first time,
I'm glad you saw the proper way to do it.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

>  dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/dump/content.c b/dump/content.c
> index c11d9b4..c248e74 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -511,6 +511,55 @@ static bool_t create_inv_session(
>  		ix_t subtreecnt,
>  		size_t strmix);
>  
> +static bool_t
> +check_rootdir(int fd,
> +	      xfs_ino_t ino)
> +{
> +	struct dirent	*gdp;
> +	size_t		gdsz;
> +	bool_t		found = BOOL_FALSE;
> +
> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
> +	if (gdsz < GETDENTSBUF_SZ_MIN)
> +		gdsz = GETDENTSBUF_SZ_MIN;
> +	gdp = (struct dirent *)calloc(1, gdsz);
> +	assert(gdp);
> +
> +	while (1) {
> +		struct dirent *p;
> +		int nread;
> +
> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> +		/*
> +		 * negative count indicates something very bad happened;
> +		 * try to gracefully end this dir.
> +		 */
> +		if (nread < 0) {
> +			mlog(MLOG_NORMAL | MLOG_WARNING,
> +_("unable to read dirents for directory ino %llu: %s\n"),
> +			      ino, strerror(errno));
> +			break;
> +		}
> +
> +		/* no more directory entries: break; */
> +		if (!nread)
> +			break;
> +
> +		for (p = gdp; nread > 0;
> +		     nread -= (int)p->d_reclen,
> +		     assert(nread >= 0),
> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> +			if (!strcmp(p->d_name, "..")) {
> +				if (p->d_ino == ino)
> +					found = BOOL_TRUE;
> +				break;
> +			}
> +		}
> +	}
> +	free(gdp);
> +	return found;
> +}
> +
>  bool_t
>  content_init(int argc,
>  	      char *argv[],
> @@ -1393,6 +1442,14 @@ baseuuidbypass:
>  			      mntpnt);
>  			return BOOL_FALSE;
>  		}
> +
> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> +			mlog(MLOG_ERROR,
> +_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
> +			     mntpnt);
> +			return BOOL_FALSE;
> +		}
> +
>  		sc_rootxfsstatp =
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets"
  2020-11-03  2:33 [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets" Gao Xiang
  2020-11-03  2:33 ` [PATCH v2 2/2] xfsdump: intercept bind mount targets Gao Xiang
  2021-06-03 20:29 ` [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets" Eric Sandeen
@ 2021-09-27 15:05 ` Bill O'Donnell
  2 siblings, 0 replies; 9+ messages in thread
From: Bill O'Donnell @ 2021-09-27 15:05 UTC (permalink / raw)
  To: Gao Xiang; +Cc: linux-xfs, Darrick J. Wong, Eric Sandeen

On Tue, Nov 03, 2020 at 10:33:14AM +0800, Gao Xiang wrote:
> Bind mount mntpnts will be forbided in the next commits
> instead since it's not the real rootdir.
> 
> This cannot be reverted cleanly due to several cleanup
> patches, but the logic is reverted equivalently.
> 
> This reverts commit 25195ebf107dc81b1b7cea1476764950e1d6cc9d.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  dump/content.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/dump/content.c b/dump/content.c
> index 30232d4..c11d9b4 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -1382,17 +1382,10 @@ baseuuidbypass:
>  	}
>  
>  	/* figure out the ino for the root directory of the fs
> -	 * and get its struct xfs_bstat for inomap_build().  This could
> -	 * be a bind mount; don't ask for the mount point inode,
> -	 * find the actual lowest inode number in the filesystem.
> +	 * and get its xfs_bstat_t for inomap_build()
>  	 */
>  	{
>  		stat64_t rootstat;
> -		xfs_ino_t lastino = 0;
> -		int ocount = 0;
> -		struct xfs_fsop_bulkreq bulkreq;
> -
> -		/* Get the inode of the mount point */
>  		rval = fstat64(sc_fsfd, &rootstat);
>  		if (rval) {
>  			mlog(MLOG_NORMAL, _(
> @@ -1404,21 +1397,11 @@ baseuuidbypass:
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
>  
> -		/* Get the first valid (i.e. root) inode in this fs */
> -		bulkreq.lastip = (__u64 *)&lastino;
> -		bulkreq.icount = 1;
> -		bulkreq.ubuffer = sc_rootxfsstatp;
> -		bulkreq.ocount = &ocount;
> -		if (ioctl(sc_fsfd, XFS_IOC_FSBULKSTAT, &bulkreq) < 0) {
> +		if (bigstat_one(sc_fsfd, rootstat.st_ino, sc_rootxfsstatp) < 0) {
>  			mlog(MLOG_ERROR,
>  			      _("failed to get bulkstat information for root inode\n"));
>  			return BOOL_FALSE;
>  		}
> -
> -		if (sc_rootxfsstatp->bs_ino != rootstat.st_ino)
> -			mlog (MLOG_NORMAL | MLOG_NOTE,
> -			       _("root ino %lld differs from mount dir ino %lld, bind mount?\n"),
> -			         sc_rootxfsstatp->bs_ino, rootstat.st_ino);
>  	}
>  
>  	/* alloc a file system handle, to be used with the jdm_open()
> -- 
> 2.18.1
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 2/2] xfsdump: intercept bind mount targets
  2020-11-03 15:33   ` [PATCH v3 " Gao Xiang
  2021-06-03 20:53     ` Eric Sandeen
@ 2021-09-27 15:07     ` Bill O'Donnell
  1 sibling, 0 replies; 9+ messages in thread
From: Bill O'Donnell @ 2021-09-27 15:07 UTC (permalink / raw)
  To: Gao Xiang; +Cc: linux-xfs, Darrick J. Wong, Eric Sandeen

On Tue, Nov 03, 2020 at 11:33:28PM +0800, Gao Xiang wrote:
> It's a bit strange pointing at some non-root bind mount target and
> then actually dumping from the actual root dir instead.
> 
> Therefore, instead of searching for the root dir of the filesystem,
> just intercept all bind mount targets by checking whose ino # of
> ".." is itself with getdents.
> 
> Fixes: 25195ebf107d ("xfsdump: handle bind mount targets")
> Cc: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
> changes since v2 (Eric):
>  - error out the case where the directory cannot be read;
>  - In any case, stop as soon as we have found "..";
>  - update the mountpoint error message and use i18n instead;
> 
>  dump/content.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/dump/content.c b/dump/content.c
> index c11d9b4..c248e74 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -511,6 +511,55 @@ static bool_t create_inv_session(
>  		ix_t subtreecnt,
>  		size_t strmix);
>  
> +static bool_t
> +check_rootdir(int fd,
> +	      xfs_ino_t ino)
> +{
> +	struct dirent	*gdp;
> +	size_t		gdsz;
> +	bool_t		found = BOOL_FALSE;
> +
> +	gdsz = sizeof(struct dirent) + NAME_MAX + 1;
> +	if (gdsz < GETDENTSBUF_SZ_MIN)
> +		gdsz = GETDENTSBUF_SZ_MIN;
> +	gdp = (struct dirent *)calloc(1, gdsz);
> +	assert(gdp);
> +
> +	while (1) {
> +		struct dirent *p;
> +		int nread;
> +
> +		nread = getdents_wrap(fd, (char *)gdp, gdsz);
> +		/*
> +		 * negative count indicates something very bad happened;
> +		 * try to gracefully end this dir.
> +		 */
> +		if (nread < 0) {
> +			mlog(MLOG_NORMAL | MLOG_WARNING,
> +_("unable to read dirents for directory ino %llu: %s\n"),
> +			      ino, strerror(errno));
> +			break;
> +		}
> +
> +		/* no more directory entries: break; */
> +		if (!nread)
> +			break;
> +
> +		for (p = gdp; nread > 0;
> +		     nread -= (int)p->d_reclen,
> +		     assert(nread >= 0),
> +		     p = (struct dirent *)((char *)p + p->d_reclen)) {
> +			if (!strcmp(p->d_name, "..")) {
> +				if (p->d_ino == ino)
> +					found = BOOL_TRUE;
> +				break;
> +			}
> +		}
> +	}
> +	free(gdp);
> +	return found;
> +}
> +
>  bool_t
>  content_init(int argc,
>  	      char *argv[],
> @@ -1393,6 +1442,14 @@ baseuuidbypass:
>  			      mntpnt);
>  			return BOOL_FALSE;
>  		}
> +
> +		if (!check_rootdir(sc_fsfd, rootstat.st_ino)) {
> +			mlog(MLOG_ERROR,
> +_("%s is not the root of the filesystem (bind mount?) - use primary mountpoint\n"),
> +			     mntpnt);
> +			return BOOL_FALSE;
> +		}
> +
>  		sc_rootxfsstatp =
>  			(struct xfs_bstat *)calloc(1, sizeof(struct xfs_bstat));
>  		assert(sc_rootxfsstatp);
> -- 
> 2.18.1
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-09-27 15:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03  2:33 [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets" Gao Xiang
2020-11-03  2:33 ` [PATCH v2 2/2] xfsdump: intercept bind mount targets Gao Xiang
2020-11-03 15:03   ` Eric Sandeen
2020-11-03 15:18     ` Gao Xiang
2020-11-03 15:33   ` [PATCH v3 " Gao Xiang
2021-06-03 20:53     ` Eric Sandeen
2021-09-27 15:07     ` Bill O'Donnell
2021-06-03 20:29 ` [PATCH v2 1/2] xfsdump: Revert "xfsdump: handle bind mount targets" Eric Sandeen
2021-09-27 15:05 ` Bill O'Donnell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.