On Fri, Feb 10, 2017 at 03:46:08AM -0500, lukeshu@lukeshu.com wrote:

From: Luke Shumaker

Motivation:

When installing the necessaryssary dependencies in the chroot, the ALPM hooks run; and if 'systemd' is a dependency, then one of the hooks is to run systemd-tmpfiles. There are several tmpfiles.d(5) commands that instruct it to create btrfs subvolumes if on btrfs (the `v`, `q`, and `Q` commands).

This causes a problem when we go to delete the chroot. The command `btrfs subvolume delete` won't recursively delete subvolumes; if a child subvolume was created, it will fail with the fairly unhelpful error message "directory not empty".

Solution:

Because the subvolume that gets mounted isn't necessarily the toplevel subvolume, and `btrfs subvolume list` gives us paths relative to the toplevel; we need to figure out how our path relates to the toplevel. Figure out the mountpoint (which turns out to be slightly tricky; see below), and call `btrfs subvolume list -a` on it to get the list of subvolumes that are visible to us (and quite possibly some that aren't; the logic for determining which ones it shows is... absurd). This gives us a list of subvolumes with numeric IDs, and paths relative to the toplevel (actually it gives us more than that, and we use a hopefully-correct `sed` expression to trim it down; the format certainly isn't human-friendly, but it's not machine-friendly either.) So then we look at that list of pairs and find the one that matches the ID of the subvolume we're trying to delete (which is easy to get with `btrfs subvolume show`); once we've found the path of our subvolume, we can use that to filter and trim the complete list of paths. From there the remainder of the solution is obvious.

Now, back to "figure out the mountpoint"; the normal `stat -c %m` doesn't work. It gives the mounted path of the subvolume closest to the path we give it, not the actual mountpoint. Now, it turns out that `df` can figure out the correct mountpoint (though I haven't investigated how it knows when stat doesn't; but I suspect it parses `/proc/mounts`). So we are reduced to parsing `df`'s output. ---

If you need to write a commit message this long just to remove what's currently a single nested subvolume, then I agree with Jan that we need to rethink our chroot management, or btrfs needs to improve its tooling (I tend to think the latter is true independent of this patch). That said, this patch does seem to work as of this writing. I have concerns that it's a little fragile and might break in the future.

makechrootpkg.in | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/makechrootpkg.in b/makechrootpkg.in index 5c4b530..01e9e96 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -80,6 +80,61 @@ load_vars() { return 0 }

+# Usage: btrfs_subvolume_id $SUBVOLUME +btrfs_subvolume_id() ( + set -o pipefail + LC_ALL=C btrfs subvolume show "$1" | sed -n 's/^\tSubvolume ID:\s*//p'

This looks like you're parsing human readable output to get the subvol ID. Is this really the only way to get this information?

+) + +# Usage: btrfs_subvolume_list_all $FILEPATH +# +# Given $FILEPATH somewhere on a mounted btrfs filesystem, print the +# ID and full path of every subvolume on the filesystem, one per line +# in the format "$ID $PATH". +btrfs_subvolume_list_all() ( + set -o pipefail + local mountpoint + mountpoint="$(df --output=target "$1" | sed 1d)" || return $?

"return $?" is a long winded way of just "return". The status is implied.

+ LC_ALL=C btrfs subvolume list -a "$mountpoint" | sed -r 's|^ID ([0-9]+) .* path (/)?(\S*).*|\1 \3|'

You might want to validate that you actually get an integer back here, rather than just assuming the calls succeed. You cannot rely on sed to provide a useful exit status.

+) + +# Usage: btrfs_subvolume_list $SUBVOLUME +# +# Assuming that $SUBVOLUME is a btrfs subvolume, list all child +# subvolumes; from most deeply nested to most shallowly nested. +# +# This is intended to be a sane version of `btrfs subvolume list`. +btrfs_subvolume_list() { + local subvolume=$1 + + local id all path subpath + id="$(btrfs_subvolume_id "$subvolume")" || return $? + all="$(btrfs_subvolume_list_all "$subvolume")" || return $? + path="$(sed -n "s/^$id //p" <<<"$all")"

Rather than injecting unvalidated data into a sed program, I'd suggest using awk: path=$(awk -v id="$1" '$1 == id { sub($1 FS, ""); print }' <<<"$all")

+ while read -r id subpath; do + if [[ "$subpath" = "$path"/* ]]; then + printf '%s\n' "${subpath#"${path}/"}" + fi + done <<<"$all" | LC_ALL=C sort --reverse +} + +# Usage: btrfs_subvolume_delete $SUBVOLUME +# +# Assuming that $SUBVOLUME is a btrfs subvolume, delete it and all +# subvolumes below it. +# +# This is intended to be a recursive version of +# `btrfs subvolume delete`. +btrfs_subvolume_delete() { + local dir="$1" + local subvolumes subvolume + subvolumes=($(btrfs_subvolume_list "$dir")) || return $?

This is a broken way to read lines into an array, because you're not reading lines at all -- you're reading words.

+ for subvolume in "${subvolumes[@]}"; do + btrfs subvolume delete "$dir/$subvolume" || return $? + done + btrfs subvolume delete "$dir" +} + create_chroot() { # Lock the chroot we want to use. We'll keep this lock until we exit. lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy" @@ -92,7 +147,7 @@ create_chroot() { stat_busy "Creating clean working copy [%s]" "$copy" if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then if [[ -d $copydir ]]; then - btrfs subvolume delete "$copydir" >/dev/null || + btrfs_subvolume_delete "$copydir" >/dev/null || die "Unable to delete subvolume %s" "$copydir" fi btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null || @@ -114,7 +169,7 @@ create_chroot() { clean_temporary() { stat_busy "Removing temporary copy [%s]" "$copy" if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then - btrfs subvolume delete "$copydir" >/dev/null || + btrfs_subvolume_delete "$copydir" >/dev/null || die "Unable to delete subvolume %s" "$copydir" else # avoid change of filesystem in case of an umount failure -- 2.11.1

From: Luke Shumaker Motivation: tmpfiles.d(5) has directives to create btrfs subvolumes. This means that systemd-tmpfiles (which may be called by an ALPM hook) might create subvolumes. For instance, systemd's systemd-nspawn.conf creates a subvolume at `/var/lib/machines/`. This causes a problem when we go to delete the chroot. The command `btrfs subvolume delete` won't recursively delete subvolumes; if a child subvolume was created, it will fail with the fairly unhelpful error message "directory not empty". Solution: Because the subvolume that gets mounted isn't necessarily the toplevel subvolume, and `btrfs subvolume list` gives us paths relative to the toplevel; we need to figure out how our path relates to the toplevel. Figure out the mountpoint (which turns out to be slightly tricky; see below), and call `btrfs subvolume list -a` on it to get the list of subvolumes that are visible to us (and quite possibly some that aren't; the logic for determining which ones it shows is... absurd). This gives us a list of subvolumes with numeric IDs, and paths relative to the toplevel (actually it gives us more than that, and we use a hopefully-correct `sed` expression to trim it down) So then we look at that list of pairs and find the one that matches the ID of the subvolume we're trying to delete (which is easy to get with `btrfs subvolume show`); once we've found the path of our subvolume, we can use that to filter and trim the complete list of paths. From there the remainder of the solution is obvious. Now, back to "figure out the mountpoint"; the normal `stat -c %m` doesn't work. It gives the mounted path of the subvolume closest to the path we give it, not the actual mountpoint. Now, it turns out that `df` can figure out the correct mountpoint (though I haven't investigated how it knows when stat doesn't; but I suspect it parses `/proc/mounts`). So we are reduced to parsing `df`'s output. Now, back to "hopefully-correct `sed` expression"; the output of `btrfs subvolume list -a` is a space-separated sequence of "key value key value...". Unfortunately both keys and values can contain space, and there's no escaping or indication of when this happens. With how we choose to parse it, a path containing a space is truncated at the first space. This means that at least the prefix is correct; if a path gets mangled, it just means that the deletion fails. As "path" is (currently) the last key, it seems tempting to allow it to simply run until the end of the line. However, this creates the possibility of a path containing " path ", which would cause the *prefix* to be trimmed, which means our failure case is now unpredictable, and potentially harmful. While we pretty much trust the user, that's still scary. --- makechrootpkg.in | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 5c4b530..ef90a68 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -80,6 +80,86 @@ load_vars() { return 0 } +# Usage: btrfs_subvolume_id $SUBVOLUME +btrfs_subvolume_id() ( + set -o pipefail + LC_ALL=C btrfs subvolume show "$1" | sed -n 's/^\tSubvolume ID:\s*//p' +) + +# Usage: btrfs_subvolume_list_all $FILEPATH +# +# Given $FILEPATH somewhere on a mounted btrfs filesystem, print the +# ID and full path of every subvolume on the filesystem, one per line +# in the format "$ID $PATH", where $PATH is relative to the top-level +# subvolume (which might not be what is mounted). +# +# BUG: Due to limitations in the `btrfs` tool, this will not correctly +# list subvolumes whose path contains a space. +btrfs_subvolume_list_all() ( + set -o pipefail + + local mountpoint all + mountpoint="$(df --output=target "$1" | sed 1d)" || return + # The output of `btrfs subvolume list -a` is a space-separated + # sequence of "key value key value...". Unfortunately, both + # keys and values can contain space, and there's no escaping + # or indication of when this happens. So we assume + # 1. ID is the first column + # 2. That no key or value will contain " path" + # 3. That the "path" value does not contain a space. + all="$(LC_ALL=C btrfs subvolume list -a "$mountpoint" | sed -r 's|^ID ([0-9]+) .* path (/)?(\S*).*|\1 \3|')" || return + + # Sanity check the output + local id path + while read -r id path; do + # ID should be numeric + [[ "$id" =~ ^-?[0-9]+$ ]] || return + # While a path could countain a space, the above code + # doesn't support it; if there is space, then it means + # we got a line not matching the expected format. + [[ "$path" != *' '* ]] || return + done <<<"$all" + + printf '%s\n' "$all" +) + +# Usage: btrfs_subvolume_list $SUBVOLUME +# +# Assuming that $SUBVOLUME is a btrfs subvolume, list all child +# subvolumes; from most deeply nested to most shallowly nested. +# +# This is intended to be a sane version of `btrfs subvolume list`. +btrfs_subvolume_list() { + local subvolume=$1 + + local id all path subpath + id="$(btrfs_subvolume_id "$subvolume")" || return + all="$(btrfs_subvolume_list_all "$subvolume")" || return + path=$(awk -v id="$1" '$1 == id { sub($1 FS, ""); print }' <<<"$all") + while read -r id subpath; do + if [[ "$subpath" = "$path"/* ]]; then + printf '%s\n' "${subpath#"${path}/"}" + fi + done <<<"$all" | LC_ALL=C sort --reverse +} + +# Usage: btrfs_subvolume_delete $SUBVOLUME +# +# Assuming that $SUBVOLUME is a btrfs subvolume, delete it and all +# subvolumes below it. +# +# This is intended to be a recursive version of +# `btrfs subvolume delete`. +btrfs_subvolume_delete() { + local dir="$1" + local subvolumes subvolume + subvolumes="$(btrfs_subvolume_list "$dir")" || return + for read -r subvolume; do + btrfs subvolume delete "$dir/$subvolume" || return + done <<<"$subvolumes" + btrfs subvolume delete "$dir" +} + create_chroot() { # Lock the chroot we want to use. We'll keep this lock until we exit. lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy" @@ -92,7 +172,7 @@ create_chroot() { stat_busy "Creating clean working copy [%s]" "$copy" if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then if [[ -d $copydir ]]; then - btrfs subvolume delete "$copydir" >/dev/null || + btrfs_subvolume_delete "$copydir" >/dev/null || die "Unable to delete subvolume %s" "$copydir" fi btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null || @@ -114,7 +194,7 @@ create_chroot() { clean_temporary() { stat_busy "Removing temporary copy [%s]" "$copy" if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then - btrfs subvolume delete "$copydir" >/dev/null || + btrfs_subvolume_delete "$copydir" >/dev/null || die "Unable to delete subvolume %s" "$copydir" else # avoid change of filesystem in case of an umount failure -- 2.11.1

From: Luke Shumaker Motivation: tmpfiles.d(5) has directives to create btrfs subvolumes. This means that systemd-tmpfiles (which may be called by an ALPM hook) might create subvolumes. For instance, systemd's systemd-nspawn.conf creates a subvolume at `/var/lib/machines/`. This causes a problem when we go to delete the chroot. The command `btrfs subvolume delete` won't recursively delete subvolumes; if a child subvolume was created, it will fail with the fairly unhelpful error message "directory not empty". Solution: Because the subvolume that gets mounted isn't necessarily the toplevel subvolume, and `btrfs subvolume list` gives us paths relative to the toplevel; we need to figure out how our path relates to the toplevel. Figure out the mountpoint (which turns out to be slightly tricky; see below), and call `btrfs subvolume list -a` on it to get the list of subvolumes that are visible to us (and quite possibly some that aren't; the logic for determining which ones it shows is... absurd). This gives us a list of subvolumes with numeric IDs, and paths relative to the toplevel (actually it gives us more than that, and we use a hopefully-correct `sed` expression to trim it down) So then we look at that list of pairs and find the one that matches the ID of the subvolume we're trying to delete (which is easy to get with `btrfs subvolume show`); once we've found the path of our subvolume, we can use that to filter and trim the complete list of paths. From there the remainder of the solution is obvious. Now, back to "figure out the mountpoint"; the normal `stat -c %m` doesn't work. It gives the mounted path of the subvolume closest to the path we give it, not the actual mountpoint. Now, it turns out that `df` can figure out the correct mountpoint (though I haven't investigated how it knows when stat doesn't; but I suspect it parses `/proc/mounts`). So we are reduced to parsing `df`'s output. Now, back to "hopefully-correct `sed` expression"; the output of `btrfs subvolume list -a` is a space-separated sequence of "key value key value...". Unfortunately both keys and values can contain space, and there's no escaping or indication of when this happens. With how we choose to parse it, a path containing a space is truncated at the first space. This means that at least the prefix is correct; if a path gets mangled, it just means that the deletion fails. As "path" is (currently) the last key, it seems tempting to allow it to simply run until the end of the line. However, this creates the possibility of a path containing " path ", which would cause the *prefix* to be trimmed, which means our failure case is now unpredictable, and potentially harmful. While we pretty much trust the user, that's still scary. --- makechrootpkg.in | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 2 deletions(-) diff --git a/makechrootpkg.in b/makechrootpkg.in index 5c4b530..7d3ebed 100644 --- a/makechrootpkg.in +++ b/makechrootpkg.in @@ -80,6 +80,95 @@ load_vars() { return 0 } +# Usage: btrfs_subvolume_id $SUBVOLUME +btrfs_subvolume_id() ( + set -o pipefail + LC_ALL=C btrfs subvolume show "$1" | sed -n 's/^\tSubvolume ID:\s*//p' +) + +# Usage: btrfs_subvolume_list_all $FILEPATH +# +# Given $FILEPATH somewhere on a mounted btrfs filesystem, print the +# ID and full path of every subvolume on the filesystem, one per line +# in the format "$ID $PATH", where $PATH is relative to the top-level +# subvolume (which might not be what is mounted). +# +# BUG: Due to limitations in the `btrfs` tool, this will not correctly +# list subvolumes whose path contains a space. +btrfs_subvolume_list_all() ( + set -o pipefail + + local mountpoint all + mountpoint="$(df --output=target "$1" | sed 1d)" || return + # The output of `btrfs subvolume list -a` is a space-separated + # sequence of "key value key value...". Unfortunately both + # keys and values can contain space, and there's no escaping + # or indication of when this happens. So we assume + # 1. ID is the first column + # 2. That no key or value will contain " path" + # 3. That the "path" value does not contain a space. + all="$(LC_ALL=C btrfs subvolume list -a "$mountpoint" | sed -r 's|^ID ([0-9]+) .* path (/)?(\S*).*|\1 \3|')" || return + + # Sanity check the output + local id path + while read -r id path; do + # ID should be numeric + [[ "$id" =~ ^-?[0-9]+$ ]] || return + # While a path could countain a space, the above code + # doesn't support it; if there is space, then it means + # we got a line not matching the expected format. + [[ "$path" != *' '* ]] || return + done <<<"$all" + + printf '%s\n' "$all" +) + +# Usage: btrfs_subvolume_list $SUBVOLUME +# +# Assuming that $SUBVOLUME is a btrfs subvolume, list all child +# subvolumes; from most deeply nested to most shallowly nested. +# +# This is intended to be a sane version of `btrfs subvolume list`. +btrfs_subvolume_list() { + local subvolume=$1 + + local id all path subpath + id="$(btrfs_subvolume_id "$subvolume")" || return + all="$(btrfs_subvolume_list_all "$subvolume")" || return + path=$(awk -v id="$id" '$1 == id { sub($1 FS, ""); print }' <<<"$all") + while read -r id subpath; do + if [[ "$subpath" = "$path"/* ]]; then + printf '%s\n' "${subpath#"${path}/"}" + fi + done <<<"$all" | LC_ALL=C sort --reverse +} + +# Usage: btrfs_subvolume_delete $SUBVOLUME +# +# Assuming that $SUBVOLUME is a btrfs subvolume, delete it and all +# subvolumes below it. +# +# This is intended to be a recursive version of +# `btrfs subvolume delete`. +btrfs_subvolume_delete() { + local dir="$1" + + # We store the result as a variable because we want to see if + # btrfs_subvolume_list fails or succeeds before we start + # deleting things. (Then we have to work around the subshell + # trimming the trailing newlines.) + local subvolumes + subvolumes="$(btrfs_subvolume_list "$dir")" || return + [[ -z "$subvolumes" ]] || subvolumes+=$'\n' + + local subvolume + while read -r subvolume; do + btrfs subvolume delete "$dir/$subvolume" || return + done < <(printf '%s' "$subvolumes") + + btrfs subvolume delete "$dir" +} + create_chroot() { # Lock the chroot we want to use. We'll keep this lock until we exit. lock 9 "$copydir.lock" "Locking chroot copy [%s]" "$copy" @@ -92,7 +181,7 @@ create_chroot() { stat_busy "Creating clean working copy [%s]" "$copy" if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then if [[ -d $copydir ]]; then - btrfs subvolume delete "$copydir" >/dev/null || + btrfs_subvolume_delete "$copydir" >/dev/null || die "Unable to delete subvolume %s" "$copydir" fi btrfs subvolume snapshot "$chrootdir/root" "$copydir" >/dev/null || @@ -114,7 +203,7 @@ create_chroot() { clean_temporary() { stat_busy "Removing temporary copy [%s]" "$copy" if [[ "$chroottype" == btrfs ]] && ! mountpoint -q "$copydir"; then - btrfs subvolume delete "$copydir" >/dev/null || + btrfs_subvolume_delete "$copydir" >/dev/null || die "Unable to delete subvolume %s" "$copydir" else # avoid change of filesystem in case of an umount failure -- 2.11.1