Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 2 Mar 2020 15:30:43 +0100
From:   Carlos Maiolino <cmaiolino@redhat.com>
To:     Lukas Czerner <lczerner@redhat.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        Zdenek Kabelac <zkabelac@redhat.com>,
        Karel Zak <kzak@redhat.com>
Subject: Re: [PATCH] libext2fs/ismounted.c: check open(O_EXCL) before mntent
 file
Message-ID: <20200302143043.h2p3zpbf6t4qfkxk@andromeda>
Mail-Followup-To: Lukas Czerner <lczerner@redhat.com>,
        linux-ext4@vger.kernel.org, tytso@mit.edu,
        Zdenek Kabelac <zkabelac@redhat.com>, Karel Zak <kzak@redhat.com>
References: <20200225143445.13182-1-lczerner@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200225143445.13182-1-lczerner@redhat.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 25, 2020 at 03:34:45PM +0100, Lukas Czerner wrote:
> Currently the ext2fs_check_mount_point() will use the open(O_EXCL) check
> on linux after all the other checks are done. However it is not
> necessary to check mntent if open(O_EXCL) succeeds because it means that
> the device is not mounted.
> 
> Moreover the commit ea4d53b7 introduced a regression where a following
> set of commands fails:
> 
> vgcreate mygroup /dev/sda
> lvcreate -L 1G -n lvol0 mygroup
> mkfs.ext4 /dev/mygroup/lvol0
> mount /dev/mygroup/lvol0 /mnt
> lvrename /dev/mygroup/lvol0 /dev/mygroup/lvol1
> lvcreate -L 1G -n lvol0 mygroup
> mkfs.ext4 /dev/mygroup/lvol0   <<<--- This fails
> 
> It fails because it thinks that /dev/mygroup/lvol0 is mounted because
> the device name in /proc/mounts is not updated following the lvrename.
> 
> Move the open(O_EXCL) check before the mntent check and return
> immediatelly if the device is not busy.
> 
> Fixes: ea4d53b7 ("libext2fs/ismounted.c: check device id in advance to skip false device names")
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
> Reported-by: Karel Zak <kzak@redhat.com>
> ---

...

> +			if (fd >= 0) {
> +				/*
> +				 * The device is not busy so it's
> +				 * definitelly not mounted. No need to
> +				 * to perform any more checks.
> +				 */
> +				close(fd);
> +				*mount_flags = 0;
> +				return 0;
> +			} else if (errno == EBUSY)
> +				busy = 1;

			^ I think you are supposed to use {} for the 'else if'
			  branch too, once the first branch uses it

Other than the small coding style above, which depends on maintainer, the code
looks ok, and you can add:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

Cheers


> +		}
> +	}
> +#endif
> +
>  	if (is_swap_device(device)) {
>  		*mount_flags = EXT2_MF_MOUNTED | EXT2_MF_SWAP;
>  		strncpy(mtpt, "<swap>", mtlen);
> @@ -386,21 +410,8 @@ errcode_t ext2fs_check_mount_point(const char *device, int *mount_flags,
>  	if (retval)
>  		return retval;
>  
> -#ifdef __linux__ /* This only works on Linux 2.6+ systems */
> -	{
> -		struct stat st_buf;
> -
> -		if (stat(device, &st_buf) == 0 &&
> -		    ext2fsP_is_disk_device(st_buf.st_mode)) {
> -			int fd = open(device, O_RDONLY | O_EXCL);
> -
> -			if (fd >= 0)
> -				close(fd);
> -			else if (errno == EBUSY)
> -				*mount_flags |= EXT2_MF_BUSY;
> -		}
> -	}
> -#endif
> +	if (busy)
> +		*mount_flags |= EXT2_MF_BUSY;
>  
>  	return 0;
>  }
> -- 
> 2.21.1
> 

-- 
Carlos