[PATCH] xfsprogs: be careful about what we stat in platform_check_mount

[PATCH] xfsprogs: be careful about what we stat in platform_check_mount

[Eric Sandeen]

After we lost ustat(2) in commit 4e7a824, we ended up with a slightly
bonkers method to determine if our target block device was mounted:
it goes through every entry returned by getmntent and stats the dir
to see if its underlying device matches ours.

Unfortunately that dir might be a hung nfs server and sadness ensues.

So do some pre-checks; can we stat the mounted "device?"  If so is
it really a block device?  If not, skip it.

Fixes: 4e7a824 ("libxfs/linux.c: Replace use of ustat by stat")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/libxfs/linux.c b/libxfs/linux.c
index 0bace3e..7350fa7 100644
--- a/libxfs/linux.c
+++ b/libxfs/linux.c
@@ -77,7 +77,21 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
 		    progname, name);
 		return 1;
 	}
+	/*
+	 * This whole business is to work out if our block device is mounted
+	 * after we lost ustat(2), see:
+	 * 	4e7a824 libxfs/linux.c: Replace use of ustat by stat
+	 * We don't really want to stat every single mounted directory,
+	 * as that may include tmpfs, cgroups, procfs or - worst - hung nfs
+	 * servers.  So we check the "device" before going after the mountpoint.
+	 */
 	while ((mnt = getmntent(f)) != NULL) {
+		/* If the "fsname" is't a stat-able device, skip it */
+		if (stat(mnt->mnt_fsname, &mst) < 0)
+			continue;
+		if (!S_ISBLK(mst.st_mode))
+			continue;
+		/* Ok, this entry does seem to be a mounted block device */
 		if (stat(mnt->mnt_dir, &mst) < 0)
 			continue;
 		if (mst.st_dev != s->st_rdev)

Re: [PATCH] xfsprogs: be careful about what we stat in platform_check_mount

[Darrick J. Wong]

On Wed, May 02, 2018 at 02:56:22PM -0500, Eric Sandeen wrote:
> After we lost ustat(2) in commit 4e7a824, we ended up with a slightly
> bonkers method to determine if our target block device was mounted:
> it goes through every entry returned by getmntent and stats the dir
> to see if its underlying device matches ours.
> 
> Unfortunately that dir might be a hung nfs server and sadness ensues.
> 
> So do some pre-checks; can we stat the mounted "device?"  If so is
> it really a block device?  If not, skip it.
> 
> Fixes: 4e7a824 ("libxfs/linux.c: Replace use of ustat by stat")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/libxfs/linux.c b/libxfs/linux.c
> index 0bace3e..7350fa7 100644
> --- a/libxfs/linux.c
> +++ b/libxfs/linux.c
> @@ -77,7 +77,21 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
>  		    progname, name);
>  		return 1;
>  	}
> +	/*
> +	 * This whole business is to work out if our block device is mounted
> +	 * after we lost ustat(2), see:
> +	 * 	4e7a824 libxfs/linux.c: Replace use of ustat by stat
> +	 * We don't really want to stat every single mounted directory,
> +	 * as that may include tmpfs, cgroups, procfs or - worst - hung nfs
> +	 * servers.  So we check the "device" before going after the mountpoint.
> +	 */
>  	while ((mnt = getmntent(f)) != NULL) {
> +		/* If the "fsname" is't a stat-able device, skip it */
> +		if (stat(mnt->mnt_fsname, &mst) < 0)
> +			continue;
> +		if (!S_ISBLK(mst.st_mode))
> +			continue;
> +		/* Ok, this entry does seem to be a mounted block device */
>  		if (stat(mnt->mnt_dir, &mst) < 0)
>  			continue;
>  		if (mst.st_dev != s->st_rdev)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfsprogs: be careful about what we stat in platform_check_mount

[Dave Chinner]

On Wed, May 02, 2018 at 02:56:22PM -0500, Eric Sandeen wrote:
> After we lost ustat(2) in commit 4e7a824, we ended up with a slightly
> bonkers method to determine if our target block device was mounted:
> it goes through every entry returned by getmntent and stats the dir
> to see if its underlying device matches ours.
> 
> Unfortunately that dir might be a hung nfs server and sadness ensues.
> 
> So do some pre-checks; can we stat the mounted "device?"  If so is
> it really a block device?  If not, skip it.
> 
> Fixes: 4e7a824 ("libxfs/linux.c: Replace use of ustat by stat")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/libxfs/linux.c b/libxfs/linux.c
> index 0bace3e..7350fa7 100644
> --- a/libxfs/linux.c
> +++ b/libxfs/linux.c
> @@ -77,7 +77,21 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
>  		    progname, name);
>  		return 1;
>  	}
> +	/*
> +	 * This whole business is to work out if our block device is mounted
> +	 * after we lost ustat(2), see:
> +	 * 	4e7a824 libxfs/linux.c: Replace use of ustat by stat
> +	 * We don't really want to stat every single mounted directory,
> +	 * as that may include tmpfs, cgroups, procfs or - worst - hung nfs
> +	 * servers.  So we check the "device" before going after the mountpoint.
> +	 */
>  	while ((mnt = getmntent(f)) != NULL) {
> +		/* If the "fsname" is't a stat-able device, skip it */
> +		if (stat(mnt->mnt_fsname, &mst) < 0)
> +			continue;
> +		if (!S_ISBLK(mst.st_mode))
> +			continue;

That'll break with subvolumes - we'll be mounting regular files, not
block devices....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH V2] xfsprogs: be careful about what we stat in platform_check_mount

[Eric Sandeen]

After we lost ustat(2) in commit 4e7a824, we ended up with a slightly
bonkers method to determine if our target block device was mounted:
it goes through every entry returned by getmntent and stats the dir
to see if its underlying device matches ours.

Unfortunately that dir might be a hung nfs server and sadness ensues.

So just do a really simple sanity check before we try to stat the
mountpoint: does its device start with a / ?  If not, skip it.

Fixes: 4e7a824 ("libxfs/linux.c: Replace use of ustat by stat")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: just look for fully-qualified device paths vs expecting an actual
block device.

diff --git a/libxfs/linux.c b/libxfs/linux.c
index 0bace3e..5c8602e 100644
--- a/libxfs/linux.c
+++ b/libxfs/linux.c
@@ -77,7 +77,17 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
  		    progname, name);
  		return 1;
  	}
+	/*
+	 * This whole business is to work out if our block device is mounted
+	 * after we lost ustat(2), see:
+	 * 	4e7a824 libxfs/linux.c: Replace use of ustat by stat
+	 * We don't really want to stat every single mounted directory,
+	 * as that may include tmpfs, cgroups, procfs or - worst - hung nfs
+	 * servers.  So first, a simple check: does the "dev" start with "/" ?
+	 */
  	while ((mnt = getmntent(f)) != NULL) {
+		if (mnt->mnt_fsname[0] != '/')
+			continue;
  		if (stat(mnt->mnt_dir, &mst) < 0)
  			continue;
  		if (mst.st_dev != s->st_rdev)

Re: [PATCH V2] xfsprogs: be careful about what we stat in platform_check_mount

[Allison Henderson]

Looks ok to me.  You can add my review.

Reviewed by: Allison Henderson <allison.henderson@oracle.com>

Thanks!

On 05/22/2018 02:58 PM, Eric Sandeen wrote:
> After we lost ustat(2) in commit 4e7a824, we ended up with a slightly
> bonkers method to determine if our target block device was mounted:
> it goes through every entry returned by getmntent and stats the dir
> to see if its underlying device matches ours.
> 
> Unfortunately that dir might be a hung nfs server and sadness ensues.
> 
> So just do a really simple sanity check before we try to stat the
> mountpoint: does its device start with a / ?  If not, skip it.
> 
> Fixes: 4e7a824 ("libxfs/linux.c: Replace use of ustat by stat")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: just look for fully-qualified device paths vs expecting an actual
> block device.
> 
> diff --git a/libxfs/linux.c b/libxfs/linux.c
> index 0bace3e..5c8602e 100644
> --- a/libxfs/linux.c
> +++ b/libxfs/linux.c
> @@ -77,7 +77,17 @@ platform_check_mount(char *name, char *block, struct 
> stat *s, int flags)
>               progname, name);
>           return 1;
>       }
> +    /*
> +     * This whole business is to work out if our block device is mounted
> +     * after we lost ustat(2), see:
> +     *     4e7a824 libxfs/linux.c: Replace use of ustat by stat
> +     * We don't really want to stat every single mounted directory,
> +     * as that may include tmpfs, cgroups, procfs or - worst - hung nfs
> +     * servers.  So first, a simple check: does the "dev" start with "/" ?
> +     */
>       while ((mnt = getmntent(f)) != NULL) {
> +        if (mnt->mnt_fsname[0] != '/')
> +            continue;
>           if (stat(mnt->mnt_dir, &mst) < 0)
>               continue;
>           if (mst.st_dev != s->st_rdev)
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  
> https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=R6pqMHln20gGSsVL6glkacPVahf-7ntcy9uginrHVqU&s=svjEUGJ89ba2TeolXCPU9AjtORnBzV-56WGzef1Q6X0&e= 
>