[gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[Ulrich Müller]

[-- Attachment #1: Type: text/plain, Size: 7956 bytes --]

The eclass failed to remount a read-only mounted /boot, because package
collision sanity checks in recent Portage versions prevented it from
reaching pkg_preinst() at all. Furthermore, with the "mount-sandbox"
feature enabled, the mount won't be propagated past pkg_preinst() and
installed files would end up under the (shadowed) mount point.

Therefore don't even attempt to mount /boot ourselves, but error out
if it isn't mounted read/write and ask the user to mount /boot.

Also clean up and simplify. (For example, awk is a grown-up program
which doesn't need any help from egrep or sed. :-)

Closes: https://bugs.gentoo.org/532264
See-also: https://bugs.gentoo.org/274130#c5
Signed-off-by: Ulrich Müller <ulm@gentoo.org>
---
v3: Exit awk commands on first match.

v4: Added die statements after awk commands
    Fixed typo in mount-boot_is_disabled function documentation
    Reverted renaming of I_KNOW_WHAT_I_AM_DOING variable

 eclass/mount-boot.eclass | 144 +++++++++++++--------------------------
 1 file changed, 48 insertions(+), 96 deletions(-)

diff --git a/eclass/mount-boot.eclass b/eclass/mount-boot.eclass
index 938df6732f43..ca27aca7efbd 100644
--- a/eclass/mount-boot.eclass
+++ b/eclass/mount-boot.eclass
@@ -1,156 +1,108 @@
-# Copyright 1999-2015 Gentoo Foundation
+# Copyright 1999-2019 Gentoo Authors
 # Distributed under the terms of the GNU General Public License v2
 
 # @ECLASS: mount-boot.eclass
 # @MAINTAINER:
 # base-system@gentoo.org
 # @BLURB: functions for packages that install files into /boot
 # @DESCRIPTION:
 # This eclass is really only useful for bootloaders.
 #
 # If the live system has a separate /boot partition configured, then this
 # function tries to ensure that it's mounted in rw mode, exiting with an
-# error if it can't. It does nothing if /boot isn't a separate partition.
+# error if it can't.  It does nothing if /boot isn't a separate partition.
+
+case ${EAPI:-0} in
+	4|5|6|7) ;;
+	*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
+esac
 
 EXPORT_FUNCTIONS pkg_pretend pkg_preinst pkg_postinst pkg_prerm pkg_postrm
 
-# @FUNCTION: mount-boot_disabled
+# @FUNCTION: mount-boot_is_disabled
 # @INTERNAL
 # @DESCRIPTION:
 # Detect whether the current environment/build settings are such that we do not
 # want to mess with any mounts.
 mount-boot_is_disabled() {
-	# Since this eclass only deals with /boot, skip things when ROOT is active.
-	if [[ "${ROOT:-/}" != "/" ]] ; then
+	# Since this eclass only deals with /boot, skip things when EROOT is active.
+	if [[ ${EROOT:-/} != / ]] ; then
 		return 0
 	fi
 
 	# If we're only building a package, then there's no need to check things.
-	if [[ "${MERGE_TYPE}" == "buildonly" ]] ; then
+	if [[ ${MERGE_TYPE} == buildonly ]] ; then
 		return 0
 	fi
 
 	# The user wants us to leave things be.
 	if [[ -n ${DONT_MOUNT_BOOT} ]] ; then
 		return 0
 	fi
 
 	# OK, we want to handle things ourselves.
 	return 1
 }
 
 # @FUNCTION: mount-boot_check_status
 # @INTERNAL
 # @DESCRIPTION:
-# Figure out what kind of work we need to do in order to have /boot be sane.
-# Return values are:
-# 0 - Do nothing at all!
-# 1 - It's mounted, but is currently ro, so need to remount rw.
-# 2 - It's not mounted, so need to mount it rw.
+# Check if /boot is sane, i.e., mounted read/write if on a separate
+# partition.  Die if conditions are not fulfilled.
 mount-boot_check_status() {
 	# Get out fast if possible.
-	mount-boot_is_disabled && return 0
+	mount-boot_is_disabled && return
 
 	# note that /dev/BOOT is in the Gentoo default /etc/fstab file
-	local fstabstate=$(awk '!/^#|^[[:blank:]]+#|^\/dev\/BOOT/ {print $2}' /etc/fstab | egrep "^/boot$" )
-	local procstate=$(awk '$2 ~ /^\/boot$/ {print $2}' /proc/mounts)
-	local proc_ro=$(awk '{ print $2 " ," $4 "," }' /proc/mounts | sed -n '/^\/boot .*,ro,/p')
-
-	if [ -n "${fstabstate}" ] && [ -n "${procstate}" ] ; then
-		if [ -n "${proc_ro}" ] ; then
-			echo
-			einfo "Your boot partition, detected as being mounted at /boot, is read-only."
-			einfo "It will be remounted in read-write mode temporarily."
-			return 1
-		else
-			echo
-			einfo "Your boot partition was detected as being mounted at /boot."
-			einfo "Files will be installed there for ${PN} to function correctly."
-			return 0
-		fi
-	elif [ -n "${fstabstate}" ] && [ -z "${procstate}" ] ; then
-		echo
-		einfo "Your boot partition was not mounted at /boot, so it will be automounted for you."
-		einfo "Files will be installed there for ${PN} to function correctly."
-		return 2
-	else
-		echo
+	local fstabstate=$(awk '!/^[[:blank:]]*#|^\/dev\/BOOT/ && $2 == "/boot" \
+		{ print 1; exit }' /etc/fstab || die "awk failed")
+
+	if [[ -z ${fstabstate} ]] ; then
 		einfo "Assuming you do not have a separate /boot partition."
-		return 0
+		return
 	fi
-}
 
-mount-boot_pkg_pretend() {
-	# Get out fast if possible.
-	mount-boot_is_disabled && return 0
+	local procstate=$(awk '$2 == "/boot" \
+		{ print gensub(/^(.*,)?(ro|rw)(,.*)?$/, "\\2", 1, $4); exit }' \
+		/proc/mounts || die "awk failed")
 
-	elog "To avoid automounting and auto(un)installing with /boot,"
-	elog "just export the DONT_MOUNT_BOOT variable."
-	mount-boot_check_status
+	if [[ -z ${procstate} ]] ; then
+		eerror "Your boot partition is not mounted at /boot."
+		eerror "Please mount it and retry."
+		die "/boot not mounted"
+	fi
+
+	if [[ ${procstate} == ro ]] ; then
+		eerror "Your boot partition, detected as being mounted at /boot," \
+			"is read-only."
+		eerror "Please remount it read/write and retry."
+		die "/boot mounted read-only"
+	fi
+
+	einfo "Your boot partition was detected as being mounted at /boot."
+	einfo "Files will be installed there for ${PN} to function correctly."
 }
 
-mount-boot_mount_boot_partition() {
+mount-boot_pkg_pretend() {
 	mount-boot_check_status
-	case $? in
-	0)	# Nothing to do.
-		;;
-	1)	# Remount it rw.
-		mount -o remount,rw /boot
-		if [ $? -ne 0 ] ; then
-			echo
-			eerror "Unable to remount in rw mode. Please do it manually!"
-			die "Can't remount in rw mode. Please do it manually!"
-		fi
-		touch /boot/.e.remount
-		;;
-	2)	# Mount it rw.
-		mount /boot -o rw
-		if [ $? -ne 0 ] ; then
-			echo
-			eerror "Cannot automatically mount your /boot partition."
-			eerror "Your boot partition has to be mounted rw before the installation"
-			eerror "can continue. ${PN} needs to install important files there."
-			die "Please mount your /boot partition manually!"
-		fi
-		touch /boot/.e.mount
-		;;
-	esac
 }
 
 mount-boot_pkg_preinst() {
-	# Handle older EAPIs.
-	case ${EAPI:-0} in
-	[0-3]) mount-boot_pkg_pretend ;;
-	esac
-
-	mount-boot_mount_boot_partition
+	mount-boot_check_status
 }
 
 mount-boot_pkg_prerm() {
-	touch "${ROOT}"/boot/.keep 2>/dev/null
-	mount-boot_mount_boot_partition
-	touch "${ROOT}"/boot/.keep 2>/dev/null
-}
+	mount-boot_check_status
 
-mount-boot_umount_boot_partition() {
-	# Get out fast if possible.
-	mount-boot_is_disabled && return 0
-
-	if [ -e /boot/.e.remount ] ; then
-		einfo "Automatically remounting /boot as ro as it was previously."
-		rm -f /boot/.e.remount
-		mount -o remount,ro /boot
-	elif [ -e /boot/.e.mount ] ; then
-		einfo "Automatically unmounting /boot as it was previously."
-		rm -f /boot/.e.mount
-		umount /boot
+	if [[ -z ${EPREFIX} ]] \
+		&& ! ( shopt -s failglob; : "${EROOT}"/boot/.keep* ) 2>/dev/null
+	then
+		# Create a .keep file, in case it is shadowed at the mount point
+		touch "${EROOT}"/boot/.keep 2>/dev/null
 	fi
 }
 
-mount-boot_pkg_postinst() {
-	mount-boot_umount_boot_partition
-}
+# No-op phases for backwards compatibility
+mount-boot_pkg_postinst() { :; }
 
-mount-boot_pkg_postrm() {
-	mount-boot_umount_boot_partition
-}
+mount-boot_pkg_postrm() { :; }
-- 
2.24.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[Thomas Deutschmann]

[-- Attachment #1.1: Type: text/plain, Size: 248 bytes --]

Hi,

just wondering if you have seen https://bugs.gentoo.org/532264#c24. If
this is still valid, is your change really needed?


-- 
Regards,
Thomas Deutschmann / Gentoo Linux Developer
C4DD 695F A713 8F24 2AA1 5638 5849 7EE5 1D5D 74A5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 618 bytes --]

Re: [gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]

>>>>> On Sat, 07 Dec 2019, Thomas Deutschmann wrote:

> just wondering if you have seen https://bugs.gentoo.org/532264#c24.
> If this is still valid, is your change really needed?

I have. IIUC it is a special case for one ebuild and cannot be
generalised.

Ulrich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[Jaco Kroon]

Hi Ulrich,

I'm happy with this "as is", but there may be a few improvements still.

By the way:  This improves the situation for mounted ro /boot by moving
the check from preinst to pretend.

For noauto /boot (I believe the default and recommended) this fixes things.

This is the reason I decided to rather go with mounting /boot but as ro
instead of not mounting at all.

May I also suggest we start recommended read-only /boot instead of not
mounted at all in order to avoid similar issues from recurring?

Kind Regards,
Jaco

On 2019/12/07 11:10, Ulrich Müller wrote:

> The eclass failed to remount a read-only mounted /boot, because package
> collision sanity checks in recent Portage versions prevented it from
> reaching pkg_preinst() at all. Furthermore, with the "mount-sandbox"
> feature enabled, the mount won't be propagated past pkg_preinst() and
> installed files would end up under the (shadowed) mount point.
>
> Therefore don't even attempt to mount /boot ourselves, but error out
> if it isn't mounted read/write and ask the user to mount /boot.
>
> Also clean up and simplify. (For example, awk is a grown-up program
> which doesn't need any help from egrep or sed. :-)
>
> Closes: https://bugs.gentoo.org/532264
> See-also: https://bugs.gentoo.org/274130#c5
> Signed-off-by: Ulrich Müller <ulm@gentoo.org>
Acked-by: Jaco Kroon <jaco@uls.co.za>
>
> ---
> v3: Exit awk commands on first match.
>
> v4: Added die statements after awk commands
>     Fixed typo in mount-boot_is_disabled function documentation
>     Reverted renaming of I_KNOW_WHAT_I_AM_DOING variable
>
>  eclass/mount-boot.eclass | 144 +++++++++++++--------------------------
>  1 file changed, 48 insertions(+), 96 deletions(-)
>
> diff --git a/eclass/mount-boot.eclass b/eclass/mount-boot.eclass
> index 938df6732f43..ca27aca7efbd 100644
> --- a/eclass/mount-boot.eclass
> +++ b/eclass/mount-boot.eclass
> @@ -1,156 +1,108 @@
> -# Copyright 1999-2015 Gentoo Foundation
> +# Copyright 1999-2019 Gentoo Authors
>  # Distributed under the terms of the GNU General Public License v2
>  
>  # @ECLASS: mount-boot.eclass
>  # @MAINTAINER:
>  # base-system@gentoo.org
>  # @BLURB: functions for packages that install files into /boot
>  # @DESCRIPTION:
>  # This eclass is really only useful for bootloaders.
>  #
>  # If the live system has a separate /boot partition configured, then this
>  # function tries to ensure that it's mounted in rw mode, exiting with an
> -# error if it can't. It does nothing if /boot isn't a separate partition.
> +# error if it can't.  It does nothing if /boot isn't a separate
partition.
> +
> +case ${EAPI:-0} in
> +    4|5|6|7) ;;
> +    *) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
> +esac
>  
>  EXPORT_FUNCTIONS pkg_pretend pkg_preinst pkg_postinst pkg_prerm
pkg_postrm
>  
> -# @FUNCTION: mount-boot_disabled
> +# @FUNCTION: mount-boot_is_disabled
>  # @INTERNAL
>  # @DESCRIPTION:
>  # Detect whether the current environment/build settings are such that
we do not
>  # want to mess with any mounts.
>  mount-boot_is_disabled() {
> -    # Since this eclass only deals with /boot, skip things when ROOT
is active.
> -    if [[ "${ROOT:-/}" != "/" ]] ; then
> +    # Since this eclass only deals with /boot, skip things when EROOT
is active.
> +    if [[ ${EROOT:-/} != / ]] ; then
>          return 0
>      fi

I don't use spaces in path names ... but what happens here if ROOT or
EPREFIX (and by implication EROOT) contains a space?

What about just checking "${EROOT}/boot" instead?

Would that even be possible ... ?

>
>  
>      # If we're only building a package, then there's no need to check
things.
> -    if [[ "${MERGE_TYPE}" == "buildonly" ]] ; then
> +    if [[ ${MERGE_TYPE} == buildonly ]] ; then
>          return 0
>      fi
>  
>      # The user wants us to leave things be.
>      if [[ -n ${DONT_MOUNT_BOOT} ]] ; then
>          return 0
>      fi
>  
>      # OK, we want to handle things ourselves.
>      return 1
>  }
>  
>  # @FUNCTION: mount-boot_check_status
>  # @INTERNAL
>  # @DESCRIPTION:
> -# Figure out what kind of work we need to do in order to have /boot
be sane.
> -# Return values are:
> -# 0 - Do nothing at all!
> -# 1 - It's mounted, but is currently ro, so need to remount rw.
> -# 2 - It's not mounted, so need to mount it rw.
> +# Check if /boot is sane, i.e., mounted read/write if on a separate
> +# partition.  Die if conditions are not fulfilled.
>  mount-boot_check_status() {
>      # Get out fast if possible.
> -    mount-boot_is_disabled && return 0
> +    mount-boot_is_disabled && return
>  
>      # note that /dev/BOOT is in the Gentoo default /etc/fstab file
> -    local fstabstate=$(awk '!/^#|^[[:blank:]]+#|^\/dev\/BOOT/ {print
$2}' /etc/fstab | egrep "^/boot$" )
> -    local procstate=$(awk '$2 ~ /^\/boot$/ {print $2}' /proc/mounts)
> -    local proc_ro=$(awk '{ print $2 " ," $4 "," }' /proc/mounts | sed
-n '/^\/boot .*,ro,/p')
> -
> -    if [ -n "${fstabstate}" ] && [ -n "${procstate}" ] ; then
> -        if [ -n "${proc_ro}" ] ; then
> -            echo
> -            einfo "Your boot partition, detected as being mounted at
/boot, is read-only."
> -            einfo "It will be remounted in read-write mode temporarily."
> -            return 1
> -        else
> -            echo
> -            einfo "Your boot partition was detected as being mounted
at /boot."
> -            einfo "Files will be installed there for ${PN} to
function correctly."
> -            return 0
> -        fi
> -    elif [ -n "${fstabstate}" ] && [ -z "${procstate}" ] ; then
> -        echo
> -        einfo "Your boot partition was not mounted at /boot, so it
will be automounted for you."
> -        einfo "Files will be installed there for ${PN} to function
correctly."
> -        return 2
> -    else
> -        echo
> +    local fstabstate=$(awk '!/^[[:blank:]]*#|^\/dev\/BOOT/ && $2 ==
"/boot" \
> +        { print 1; exit }' /etc/fstab || die "awk failed")
> +
> +    if [[ -z ${fstabstate} ]] ; then
>          einfo "Assuming you do not have a separate /boot partition."
> -        return 0
> +        return
>      fi
> -}
>  
> -mount-boot_pkg_pretend() {
> -    # Get out fast if possible.
> -    mount-boot_is_disabled && return 0
> +    local procstate=$(awk '$2 == "/boot" \
> +        { print gensub(/^(.*,)?(ro|rw)(,.*)?$/, "\\2", 1, $4); exit }' \
> +        /proc/mounts || die "awk failed")
>  
> -    elog "To avoid automounting and auto(un)installing with /boot,"
> -    elog "just export the DONT_MOUNT_BOOT variable."
> -    mount-boot_check_status
> +    if [[ -z ${procstate} ]] ; then
> +        eerror "Your boot partition is not mounted at /boot."
> +        eerror "Please mount it and retry."
> +        die "/boot not mounted"
> +    fi
> +
> +    if [[ ${procstate} == ro ]] ; then
> +        eerror "Your boot partition, detected as being mounted at
/boot," \
> +            "is read-only."
> +        eerror "Please remount it read/write and retry."
> +        die "/boot mounted read-only"
> +    fi
> +
> +    einfo "Your boot partition was detected as being mounted at /boot."
> +    einfo "Files will be installed there for ${PN} to function
correctly."
>  }
>  
> -mount-boot_mount_boot_partition() {
> +mount-boot_pkg_pretend() {
>      mount-boot_check_status
> -    case $? in
> -    0)    # Nothing to do.
> -        ;;
> -    1)    # Remount it rw.
> -        mount -o remount,rw /boot
> -        if [ $? -ne 0 ] ; then
> -            echo
> -            eerror "Unable to remount in rw mode. Please do it manually!"
> -            die "Can't remount in rw mode. Please do it manually!"
> -        fi
> -        touch /boot/.e.remount
> -        ;;
> -    2)    # Mount it rw.
> -        mount /boot -o rw
> -        if [ $? -ne 0 ] ; then
> -            echo
> -            eerror "Cannot automatically mount your /boot partition."
> -            eerror "Your boot partition has to be mounted rw before
the installation"
> -            eerror "can continue. ${PN} needs to install important
files there."
> -            die "Please mount your /boot partition manually!"
> -        fi
> -        touch /boot/.e.mount
> -        ;;
> -    esac
>  }
>  
>  mount-boot_pkg_preinst() {
> -    # Handle older EAPIs.
> -    case ${EAPI:-0} in
> -    [0-3]) mount-boot_pkg_pretend ;;
> -    esac
> -
> -    mount-boot_mount_boot_partition
> +    mount-boot_check_status
>  }
>  
>  mount-boot_pkg_prerm() {
> -    touch "${ROOT}"/boot/.keep 2>/dev/null
> -    mount-boot_mount_boot_partition
> -    touch "${ROOT}"/boot/.keep 2>/dev/null
> -}
> +    mount-boot_check_status
>  
> -mount-boot_umount_boot_partition() {
> -    # Get out fast if possible.
> -    mount-boot_is_disabled && return 0
> -
> -    if [ -e /boot/.e.remount ] ; then
> -        einfo "Automatically remounting /boot as ro as it was
previously."
> -        rm -f /boot/.e.remount
> -        mount -o remount,ro /boot
> -    elif [ -e /boot/.e.mount ] ; then
> -        einfo "Automatically unmounting /boot as it was previously."
> -        rm -f /boot/.e.mount
> -        umount /boot
> +    if [[ -z ${EPREFIX} ]] \
> +        && ! ( shopt -s failglob; : "${EROOT}"/boot/.keep* ) 2>/dev/null
> +    then
> +        # Create a .keep file, in case it is shadowed at the mount point
> +        touch "${EROOT}"/boot/.keep 2>/dev/null
>      fi
>  }
>  
> -mount-boot_pkg_postinst() {
> -    mount-boot_umount_boot_partition
> -}
> +# No-op phases for backwards compatibility
> +mount-boot_pkg_postinst() { :; }
>  
> -mount-boot_pkg_postrm() {
> -    mount-boot_umount_boot_partition
> -}
> +mount-boot_pkg_postrm() { :; }

Re: [gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

>>>>> On Mon, 09 Dec 2019, Jaco Kroon wrote:

> Acked-by: Jaco Kroon <jaco@uls.co.za>

OK, I can add this.

>> +    if [[ ${EROOT:-/} != / ]] ; then
>>          return 0
>>      fi

> I don't use spaces in path names ... but what happens here if ROOT or
> EPREFIX (and by implication EROOT) contains a space?

No problem here, the test would still succeed and return 0. (But I
predict that EROOT containing spaces will cause a lot of trouble
elsewhere. :)

> What about just checking "${EROOT}/boot" instead?

For what, existence? There may well be a "boot" directory present under
EROOT. (And we could check ${EROOT}/etc/fstab, but I don't think we
should open that can of worms. There's no reliable way to guess the
user's exact configuration for non-trivial ROOT.)

Ulrich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[Jaco Kroon]

Hi,

>> What about just checking "${EROOT}/boot" instead?
> For what, existence? There may well be a "boot" directory present under
> EROOT. (And we could check ${EROOT}/etc/fstab, but I don't think we
> should open that can of worms. There's no reliable way to guess the
> user's exact configuration for non-trivial ROOT.)

I was hoping there is a somewhat reliable way ... but if not I guess as
is will be the best we can do.

IMHO:  Ship it.

Even if there is issues still, the new state will be better than current.

Kind Regards,
Jaco

Re: [gentoo-dev] [PATCH v4] mount-boot.eclass: Check if /boot is sane, but don't try to mount it.

[Ulrich Mueller]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

>>>>> On Sat, 07 Dec 2019, Ulrich Müller wrote:

> The eclass failed to remount a read-only mounted /boot, because package
> collision sanity checks in recent Portage versions prevented it from
> reaching pkg_preinst() at all. Furthermore, with the "mount-sandbox"
> feature enabled, the mount won't be propagated past pkg_preinst() and
> installed files would end up under the (shadowed) mount point.

> Therefore don't even attempt to mount /boot ourselves, but error out
> if it isn't mounted read/write and ask the user to mount /boot.

> Also clean up and simplify. (For example, awk is a grown-up program
> which doesn't need any help from egrep or sed. :-)

> Closes: https://bugs.gentoo.org/532264
> See-also: https://bugs.gentoo.org/274130#c5
> Signed-off-by: Ulrich Müller <ulm@gentoo.org>
> ---
> v3: Exit awk commands on first match.

> v4: Added die statements after awk commands
>     Fixed typo in mount-boot_is_disabled function documentation
>     Reverted renaming of I_KNOW_WHAT_I_AM_DOING variable

Pushed (with one small spelling change in an error message).

Ulrich

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]