* [PATCH 1/2] e2scrub: fix pathname escaping across all service definitions
2023-12-15 22:55 [PATCHSET] e2scrub: fix some problems Darrick J. Wong
@ 2023-12-15 22:56 ` Darrick J. Wong
2023-12-15 22:56 ` [PATCH 2/2] e2fsprogs: don't allow udisks to automount ext4 filesystems with no prompt Darrick J. Wong
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2023-12-15 22:56 UTC (permalink / raw)
To: djwong, tytso; +Cc: linux-ext4
From: Darrick J. Wong <djwong@kernel.org>
systemd services provide an "instance name" that can be associated with
a particular invocation of a service. This allows service users to
invoke multiple copies of a service, each with a unique string. For
e2scrub, we pass the mountpoint of the filesystem as the instance name.
However, systemd services aren't supposed to have slashes in them, so
we're supposed to escape them.
The canonical escaping scheme for pathnames is defined by the
systemd-escape --path command. Unfortunately, we've been adding our own
opinionated sauce for years, to work around the fact that --path didn't
quite work right in systemd before January 2017. The special sauce is
incorrect, and we no longer care about systemd of 7 years past.
Clean up this mess by following the systemd escaping scheme throughout
the service units. Now we can use the '%f' specifier in them, which
makes things a lot less complicated.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
scrub/e2scrub@.service.in | 4 ++--
scrub/e2scrub_all.in | 20 ++++----------------
scrub/e2scrub_fail.in | 10 +++++-----
scrub/e2scrub_fail@.service.in | 4 ++--
4 files changed, 13 insertions(+), 25 deletions(-)
diff --git a/scrub/e2scrub@.service.in b/scrub/e2scrub@.service.in
index 496f8948..6425263c 100644
--- a/scrub/e2scrub@.service.in
+++ b/scrub/e2scrub@.service.in
@@ -1,5 +1,5 @@
[Unit]
-Description=Online ext4 Metadata Check for %I
+Description=Online ext4 Metadata Check for %f
OnFailure=e2scrub_fail@%i.service
Documentation=man:e2scrub(8)
@@ -16,5 +16,5 @@ User=root
IOSchedulingClass=idle
CPUSchedulingPolicy=idle
Environment=SERVICE_MODE=1
-ExecStart=@root_sbindir@/e2scrub -t %I
+ExecStart=@root_sbindir@/e2scrub -t %f
SyslogIdentifier=%N
diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index 4288b969..437f6cc2 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -146,22 +146,10 @@ ls_targets() {
fi
}
-# systemd doesn't know to do path escaping on the instance variable we pass
-# to the e2scrub service, which breaks things if there is a dash in the path
-# name. Therefore, do the path escaping ourselves if needed.
-#
-# systemd path escaping also drops the initial slash so we add that back in so
-# that log messages from the service units preserve the full path and users can
-# look up log messages using full paths. However, for "/" the escaping rules
-# do /not/ drop the initial slash, so we have to special-case that here.
+# Turn our mount path into a service name that systemd will recognize
escape_path_for_systemd() {
local path="$1"
-
- if [ "${path}" != "/" ]; then
- echo "-$(systemd-escape --path "${path}")"
- else
- echo "-"
- fi
+ systemd-escape --template 'e2scrub@.service' --path "${path}"
}
# Scrub any mounted fs on lvm by creating a snapshot and fscking that.
@@ -170,8 +158,8 @@ for tgt in "${targets[@]}"; do
# If we're not reaping and systemd is present, try invoking the
# systemd service.
if [ "${reap}" -ne 1 ] && type systemctl > /dev/null 2>&1; then
- tgt_esc="$(escape_path_for_systemd "${tgt}")"
- ${DBG} systemctl start "e2scrub@${tgt_esc}" 2> /dev/null
+ svcname="$(escape_path_for_systemd "${tgt}")"
+ ${DBG} systemctl start "${svcname}" 2> /dev/null
res=$?
if [ "${res}" -eq 0 ] || [ "${res}" -eq 1 ]; then
continue;
diff --git a/scrub/e2scrub_fail.in b/scrub/e2scrub_fail.in
index 2c0754a9..6899c47c 100644
--- a/scrub/e2scrub_fail.in
+++ b/scrub/e2scrub_fail.in
@@ -2,8 +2,8 @@
# Email logs of failed e2scrub unit runs when the systemd service fails.
-device="$1"
-test -z "${device}" && exit 0
+mntpoint="$1"
+test -z "${mntpoint}" && exit 0
if ! type sendmail > /dev/null 2>&1; then
echo "$0: sendmail program not found."
@@ -16,7 +16,7 @@ fi
hostname="$(hostname -f 2>/dev/null)"
test -z "${hostname}" && hostname="${HOSTNAME}"
-service_name="e2scrub@$(systemd-escape ${device})"
+service_name="$(systemd-escape --template "e2scrub@.service" --path "${mntpoint}")"
if test -z "${recipient}" ; then
recipient="root"
@@ -29,9 +29,9 @@ fi
(cat << ENDL
To: ${recipient}
From: ${sender}
-Subject: e2scrub failure on ${device}
+Subject: e2scrub failure on ${mntpoint}
-So sorry, the automatic e2scrub of ${device} on ${hostname} failed.
+So sorry, the automatic e2scrub of ${mntpoint} on ${hostname} failed.
A log of what happened follows:
ENDL
diff --git a/scrub/e2scrub_fail@.service.in b/scrub/e2scrub_fail@.service.in
index 4bad311b..ae65a1da 100644
--- a/scrub/e2scrub_fail@.service.in
+++ b/scrub/e2scrub_fail@.service.in
@@ -1,10 +1,10 @@
[Unit]
-Description=Online ext4 Metadata Check Failure Reporting for %I
+Description=Online ext4 Metadata Check Failure Reporting for %f
Documentation=man:e2scrub(8)
[Service]
Type=oneshot
-ExecStart=@pkglibdir@/e2scrub_fail "%I"
+ExecStart=@pkglibdir@/e2scrub_fail "%f"
User=mail
Group=mail
SupplementaryGroups=systemd-journal
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/2] e2fsprogs: don't allow udisks to automount ext4 filesystems with no prompt
2023-12-15 22:55 [PATCHSET] e2scrub: fix some problems Darrick J. Wong
2023-12-15 22:56 ` [PATCH 1/2] e2scrub: fix pathname escaping across all service definitions Darrick J. Wong
@ 2023-12-15 22:56 ` Darrick J. Wong
2023-12-31 20:39 ` [PATCH 3/2] e2scrub_fail: move executable script to /usr/libexec Darrick J. Wong
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2023-12-15 22:56 UTC (permalink / raw)
To: djwong, tytso; +Cc: linux-ext4
From: Darrick J. Wong <djwong@kernel.org>
The unending stream of syzbot bug reports and overwrought filing of CVEs
for corner case handling (i.e. things that distract from actual user
complaints) in ext4 has generated all sorts of of overheated rhetoric
about how every bug is a Serious Security Issue(tm) because anyone can
craft a malicious filesystem on a USB stick, insert the stick into a
victim machine, and mount will trigger a bug in the kernel driver that
leads to some compromise or DoS or something.
I thought that nobody would be foolish enough to automount an ext4
filesystem. What a fool I was! It turns out that udisks can be told
that it's okay to automount things, and then GNOME will do exactly that.
Including mounting mangled ext4 filesystems!
<delete angry rant about poor decisionmaking and armchair fs developers
blasting us on X while not actually doing any of the work>
Turn off /this/ idiocy by adding a udev rule to tell udisks not to
automount ext4 filesystems.
This will not stop a logged in user from unwittingly inserting a
malicious storage device and pressing [mount] and getting breached.
This is not a substitute for a thorough audit of all codebases. This is
not a substitute for lklfuse. This does not solve the general problem
of in-kernel fs drivers being a huge attack surface. I just want a
vacation from the sh*tstorm of bad ideas and threat models that I never
agreed to support.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
scrub/Makefile.in | 12 ++++++++++--
scrub/ext4.rules.in | 13 +++++++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
create mode 100644 scrub/ext4.rules.in
diff --git a/scrub/Makefile.in b/scrub/Makefile.in
index 387f6504..d0c5c11b 100644
--- a/scrub/Makefile.in
+++ b/scrub/Makefile.in
@@ -18,6 +18,7 @@ CONFFILES= e2scrub.conf
ifeq ($(HAVE_UDEV),yes)
UDEV_RULES = e2scrub.rules
+UDISKS_RULES = ext4.rules
INSTALLDIRS_TGT += installdirs-udev
INSTALL_TGT += install-udev
UNINSTALL_TGT += uninstall-udev
@@ -39,7 +40,7 @@ INSTALL_TGT += install-systemd install-libprogs
UNINSTALL_TGT += uninstall-systemd uninstall-libprogs
endif
-all:: $(PROGS) $(MANPAGES) $(CONFFILES) $(UDEV_RULES) $(SERVICE_FILES) $(CRONTABS) $(LIBPROGS)
+all:: $(PROGS) $(MANPAGES) $(CONFFILES) $(UDEV_RULES) $(UDISKS_RULES) $(SERVICE_FILES) $(CRONTABS) $(LIBPROGS)
e2scrub: $(DEP_SUBSTITUTE) e2scrub.in
$(E) " SUBST $@"
@@ -111,6 +112,10 @@ install-udev: installdirs-udev
$(ES) " INSTALL $(UDEV_RULES_DIR)/$$i"; \
$(INSTALL_DATA) $$i $(DESTDIR)$(UDEV_RULES_DIR)/96-$$i; \
done
+ $(Q) for i in $(UDISKS_RULES); do \
+ $(ES) " INSTALL $(UDEV_RULES_DIR)/$$i"; \
+ $(INSTALL_DATA) $$i $(DESTDIR)$(UDEV_RULES_DIR)/64-$$i; \
+ done
install-crond: installdirs-crond
$(Q) if test -n "$(CRONTABS)" ; then \
@@ -153,6 +158,9 @@ uninstall-udev:
for i in $(UDEV_RULES); do \
$(RM) -f $(DESTDIR)$(UDEV_RULES_DIR)/96-$$i; \
done
+ for i in $(UDISKS_RULES); do \
+ $(RM) -f $(DESTDIR)$(UDEV_RULES_DIR)/64-$$i; \
+ done
uninstall-crond:
if test -n "$(CRONTABS)" ; then \
@@ -181,7 +189,7 @@ uninstall: $(UNINSTALL_TGT)
done
clean::
- $(RM) -f $(PROGS) $(MANPAGES) $(CONFFILES) $(UDEV_RULES) $(SERVICE_FILES) $(CRONTABS) $(LIBPROGS)
+ $(RM) -f $(PROGS) $(MANPAGES) $(CONFFILES) $(UDEV_RULES) $(UDISKS_RULES) $(SERVICE_FILES) $(CRONTABS) $(LIBPROGS)
mostlyclean: clean
distclean: clean
diff --git a/scrub/ext4.rules.in b/scrub/ext4.rules.in
new file mode 100644
index 00000000..6fe5a7a8
--- /dev/null
+++ b/scrub/ext4.rules.in
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# Copyright (C) 2023 Oracle. All rights reserved.
+# Author: Darrick J. Wong <djwong@kernel.org>
+#
+# Don't let udisks automount ext4 filesystems without even asking a user.
+# This doesn't eliminate filesystems as an attack surface; it only prevents
+# evil maid attacks when all sessions are locked.
+#
+# According to http://storaged.org/doc/udisks2-api/latest/udisks.8.html,
+# supplying UDISKS_AUTO=0 here changes the HintAuto property of the block
+# device abstraction to mean "do not automatically start" (e.g. mount).
+SUBSYSTEM=="block", ENV{ID_FS_TYPE}=="ext2|ext3|ext4|ext4dev|jbd", ENV{UDISKS_AUTO}="0"
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/2] e2scrub_fail: move executable script to /usr/libexec
2023-12-15 22:55 [PATCHSET] e2scrub: fix some problems Darrick J. Wong
2023-12-15 22:56 ` [PATCH 1/2] e2scrub: fix pathname escaping across all service definitions Darrick J. Wong
2023-12-15 22:56 ` [PATCH 2/2] e2fsprogs: don't allow udisks to automount ext4 filesystems with no prompt Darrick J. Wong
@ 2023-12-31 20:39 ` Darrick J. Wong
2023-12-31 21:37 ` Neal Gompa
2024-01-10 5:57 ` [PATCH 4/2] debian: don't restart e2scrub_all when upgrading package Darrick J. Wong
2024-01-10 5:58 ` [RFC PATCH 6/2] e2scrub: skip filesystems that don't have journals Darrick J. Wong
4 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2023-12-31 20:39 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4, Neal Gompa
From: Darrick J. Wong <djwong@kernel.org>
Per FHS 3.0, non-PATH executable binaries are supposed to live under
/usr/libexec, not /usr/lib. e2scrub_fail is an executable script, so
move it to libexec in case some distro some day tries to mount /usr/lib
as noexec or something. Also, there's no reason why these scripts need
to be put under an arch-dependent path.
Cc: Neal Gompa <neal@gompa.dev>
Link: https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
MCONFIG.in | 2 +-
debian/e2fsprogs.install | 4 ++--
scrub/Makefile.in | 10 +++++-----
scrub/e2scrub_all.cron.in | 2 +-
scrub/e2scrub_fail@.service.in | 2 +-
util/subst.conf.in | 2 +-
6 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/MCONFIG.in b/MCONFIG.in
index 82c75a28..2b1872fa 100644
--- a/MCONFIG.in
+++ b/MCONFIG.in
@@ -34,7 +34,7 @@ man8dir = $(mandir)/man8
infodir = @infodir@
datadir = @datadir@
pkgconfigdir = $(libdir)/pkgconfig
-pkglibdir = $(libdir)/e2fsprogs
+pkglibexecdir = @libexecdir@/e2fsprogs
HAVE_UDEV = @have_udev@
UDEV_RULES_DIR = @pkg_udev_rules_dir@
diff --git a/debian/e2fsprogs.install b/debian/e2fsprogs.install
index 8cf07a6f..b50078d3 100755
--- a/debian/e2fsprogs.install
+++ b/debian/e2fsprogs.install
@@ -16,8 +16,8 @@ sbin/resize2fs
sbin/tune2fs
usr/bin/chattr
usr/bin/lsattr
-[linux-any] usr/lib/*/e2fsprogs/e2scrub_all_cron
-[linux-any] usr/lib/*/e2fsprogs/e2scrub_fail
+[linux-any] usr/libexec/e2fsprogs/e2scrub_all_cron
+[linux-any] usr/libexec/e2fsprogs/e2scrub_fail
usr/sbin/e2freefrag
[linux-any] usr/sbin/e4crypt
[linux-any] usr/sbin/e4defrag
diff --git a/scrub/Makefile.in b/scrub/Makefile.in
index d0c5c11b..c97a1dd5 100644
--- a/scrub/Makefile.in
+++ b/scrub/Makefile.in
@@ -95,8 +95,8 @@ installdirs-crond:
$(Q) $(MKDIR_P) $(DESTDIR)$(CROND_DIR)
installdirs-libprogs:
- $(E) " MKDIR_P $(pkglibdir)"
- $(Q) $(MKDIR_P) $(DESTDIR)$(pkglibdir)
+ $(E) " MKDIR_P $(pkglibexecdir)"
+ $(Q) $(MKDIR_P) $(DESTDIR)$(pkglibexecdir)
installdirs-systemd:
$(E) " MKDIR_P $(SYSTEMD_SYSTEM_UNIT_DIR)"
@@ -125,8 +125,8 @@ install-crond: installdirs-crond
install-libprogs: $(LIBPROGS) installdirs-libprogs
$(Q) for i in $(LIBPROGS); do \
- $(ES) " INSTALL $(pkglibdir)/$$i"; \
- $(INSTALL_PROGRAM) $$i $(DESTDIR)$(pkglibdir)/$$i; \
+ $(ES) " INSTALL $(pkglibexecdir)/$$i"; \
+ $(INSTALL_PROGRAM) $$i $(DESTDIR)$(pkglibexecdir)/$$i; \
done
install-systemd: $(SERVICE_FILES) installdirs-systemd
@@ -169,7 +169,7 @@ uninstall-crond:
uninstall-libprogs:
for i in $(LIBPROGS); do \
- $(RM) -f $(DESTDIR)$(pkglibdir)/$$i; \
+ $(RM) -f $(DESTDIR)$(pkglibexecdir)/$$i; \
done
uninstall-systemd:
diff --git a/scrub/e2scrub_all.cron.in b/scrub/e2scrub_all.cron.in
index 395fb2ab..8e2640d4 100644
--- a/scrub/e2scrub_all.cron.in
+++ b/scrub/e2scrub_all.cron.in
@@ -1,2 +1,2 @@
-30 3 * * 0 root test -e /run/systemd/system || SERVICE_MODE=1 @pkglibdir@/e2scrub_all_cron
+30 3 * * 0 root test -e /run/systemd/system || SERVICE_MODE=1 @pkglibexecdir@/e2scrub_all_cron
10 3 * * * root test -e /run/systemd/system || SERVICE_MODE=1 @root_sbindir@/e2scrub_all -A -r
diff --git a/scrub/e2scrub_fail@.service.in b/scrub/e2scrub_fail@.service.in
index ae65a1da..462daee2 100644
--- a/scrub/e2scrub_fail@.service.in
+++ b/scrub/e2scrub_fail@.service.in
@@ -4,7 +4,7 @@ Documentation=man:e2scrub(8)
[Service]
Type=oneshot
-ExecStart=@pkglibdir@/e2scrub_fail "%f"
+ExecStart=@pkglibexecdir@/e2scrub_fail "%f"
User=mail
Group=mail
SupplementaryGroups=systemd-journal
diff --git a/util/subst.conf.in b/util/subst.conf.in
index 0da45541..5af5e356 100644
--- a/util/subst.conf.in
+++ b/util/subst.conf.in
@@ -23,4 +23,4 @@ root_sbindir @root_sbindir@
root_bindir @root_bindir@
libdir @libdir@
$exec_prefix @exec_prefix@
-pkglibdir @libdir@/e2fsprogs
+pkglibexecdir @libexecdir@/e2fsprogs
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/2] e2scrub_fail: move executable script to /usr/libexec
2023-12-31 20:39 ` [PATCH 3/2] e2scrub_fail: move executable script to /usr/libexec Darrick J. Wong
@ 2023-12-31 21:37 ` Neal Gompa
0 siblings, 0 replies; 7+ messages in thread
From: Neal Gompa @ 2023-12-31 21:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: tytso, linux-ext4
On Sun, Dec 31, 2023 at 3:39 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> From: Darrick J. Wong <djwong@kernel.org>
>
> Per FHS 3.0, non-PATH executable binaries are supposed to live under
> /usr/libexec, not /usr/lib. e2scrub_fail is an executable script, so
> move it to libexec in case some distro some day tries to mount /usr/lib
> as noexec or something. Also, there's no reason why these scripts need
> to be put under an arch-dependent path.
>
> Cc: Neal Gompa <neal@gompa.dev>
> Link: https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> MCONFIG.in | 2 +-
> debian/e2fsprogs.install | 4 ++--
> scrub/Makefile.in | 10 +++++-----
> scrub/e2scrub_all.cron.in | 2 +-
> scrub/e2scrub_fail@.service.in | 2 +-
> util/subst.conf.in | 2 +-
> 6 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/MCONFIG.in b/MCONFIG.in
> index 82c75a28..2b1872fa 100644
> --- a/MCONFIG.in
> +++ b/MCONFIG.in
> @@ -34,7 +34,7 @@ man8dir = $(mandir)/man8
> infodir = @infodir@
> datadir = @datadir@
> pkgconfigdir = $(libdir)/pkgconfig
> -pkglibdir = $(libdir)/e2fsprogs
> +pkglibexecdir = @libexecdir@/e2fsprogs
>
> HAVE_UDEV = @have_udev@
> UDEV_RULES_DIR = @pkg_udev_rules_dir@
> diff --git a/debian/e2fsprogs.install b/debian/e2fsprogs.install
> index 8cf07a6f..b50078d3 100755
> --- a/debian/e2fsprogs.install
> +++ b/debian/e2fsprogs.install
> @@ -16,8 +16,8 @@ sbin/resize2fs
> sbin/tune2fs
> usr/bin/chattr
> usr/bin/lsattr
> -[linux-any] usr/lib/*/e2fsprogs/e2scrub_all_cron
> -[linux-any] usr/lib/*/e2fsprogs/e2scrub_fail
> +[linux-any] usr/libexec/e2fsprogs/e2scrub_all_cron
> +[linux-any] usr/libexec/e2fsprogs/e2scrub_fail
> usr/sbin/e2freefrag
> [linux-any] usr/sbin/e4crypt
> [linux-any] usr/sbin/e4defrag
> diff --git a/scrub/Makefile.in b/scrub/Makefile.in
> index d0c5c11b..c97a1dd5 100644
> --- a/scrub/Makefile.in
> +++ b/scrub/Makefile.in
> @@ -95,8 +95,8 @@ installdirs-crond:
> $(Q) $(MKDIR_P) $(DESTDIR)$(CROND_DIR)
>
> installdirs-libprogs:
> - $(E) " MKDIR_P $(pkglibdir)"
> - $(Q) $(MKDIR_P) $(DESTDIR)$(pkglibdir)
> + $(E) " MKDIR_P $(pkglibexecdir)"
> + $(Q) $(MKDIR_P) $(DESTDIR)$(pkglibexecdir)
>
> installdirs-systemd:
> $(E) " MKDIR_P $(SYSTEMD_SYSTEM_UNIT_DIR)"
> @@ -125,8 +125,8 @@ install-crond: installdirs-crond
>
> install-libprogs: $(LIBPROGS) installdirs-libprogs
> $(Q) for i in $(LIBPROGS); do \
> - $(ES) " INSTALL $(pkglibdir)/$$i"; \
> - $(INSTALL_PROGRAM) $$i $(DESTDIR)$(pkglibdir)/$$i; \
> + $(ES) " INSTALL $(pkglibexecdir)/$$i"; \
> + $(INSTALL_PROGRAM) $$i $(DESTDIR)$(pkglibexecdir)/$$i; \
> done
>
> install-systemd: $(SERVICE_FILES) installdirs-systemd
> @@ -169,7 +169,7 @@ uninstall-crond:
>
> uninstall-libprogs:
> for i in $(LIBPROGS); do \
> - $(RM) -f $(DESTDIR)$(pkglibdir)/$$i; \
> + $(RM) -f $(DESTDIR)$(pkglibexecdir)/$$i; \
> done
>
> uninstall-systemd:
> diff --git a/scrub/e2scrub_all.cron.in b/scrub/e2scrub_all.cron.in
> index 395fb2ab..8e2640d4 100644
> --- a/scrub/e2scrub_all.cron.in
> +++ b/scrub/e2scrub_all.cron.in
> @@ -1,2 +1,2 @@
> -30 3 * * 0 root test -e /run/systemd/system || SERVICE_MODE=1 @pkglibdir@/e2scrub_all_cron
> +30 3 * * 0 root test -e /run/systemd/system || SERVICE_MODE=1 @pkglibexecdir@/e2scrub_all_cron
> 10 3 * * * root test -e /run/systemd/system || SERVICE_MODE=1 @root_sbindir@/e2scrub_all -A -r
> diff --git a/scrub/e2scrub_fail@.service.in b/scrub/e2scrub_fail@.service.in
> index ae65a1da..462daee2 100644
> --- a/scrub/e2scrub_fail@.service.in
> +++ b/scrub/e2scrub_fail@.service.in
> @@ -4,7 +4,7 @@ Documentation=man:e2scrub(8)
>
> [Service]
> Type=oneshot
> -ExecStart=@pkglibdir@/e2scrub_fail "%f"
> +ExecStart=@pkglibexecdir@/e2scrub_fail "%f"
> User=mail
> Group=mail
> SupplementaryGroups=systemd-journal
> diff --git a/util/subst.conf.in b/util/subst.conf.in
> index 0da45541..5af5e356 100644
> --- a/util/subst.conf.in
> +++ b/util/subst.conf.in
> @@ -23,4 +23,4 @@ root_sbindir @root_sbindir@
> root_bindir @root_bindir@
> libdir @libdir@
> $exec_prefix @exec_prefix@
> -pkglibdir @libdir@/e2fsprogs
> +pkglibexecdir @libexecdir@/e2fsprogs
Looks great to me.
Reviewed-by: Neal Gompa <neal@gompa.dev>
--
真実はいつも一つ！/ Always, there's only one truth!
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4/2] debian: don't restart e2scrub_all when upgrading package
2023-12-15 22:55 [PATCHSET] e2scrub: fix some problems Darrick J. Wong
` (2 preceding siblings ...)
2023-12-31 20:39 ` [PATCH 3/2] e2scrub_fail: move executable script to /usr/libexec Darrick J. Wong
@ 2024-01-10 5:57 ` Darrick J. Wong
2024-01-10 5:58 ` [RFC PATCH 6/2] e2scrub: skip filesystems that don't have journals Darrick J. Wong
4 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2024-01-10 5:57 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4
From: Darrick J. Wong <djwong@kernel.org>
When installing or upgrading the e2fsprogs package, only start the
e2scrub_all timer and the reaping service. Don't restart e2scrub_all
itself, because that will kill any scrubs in progress, which will
trigger the failure reporting.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
debian/rules | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/debian/rules b/debian/rules
index b85976f0..0120fe6a 100755
--- a/debian/rules
+++ b/debian/rules
@@ -148,6 +148,10 @@ override_dh_installinfo:
dh_installinfo -pcomerr-dev ${stdbuilddir}/lib/et/com_err.info
dh_installinfo -plibext2fs-dev ${stdbuilddir}/doc/libext2fs.info
+override_dh_installsystemd:
+ dh_installsystemd -p e2fsprogs --no-restart-after-upgrade --no-stop-on-upgrade e2scrub_all.timer e2scrub_reap.service
+ dh_installsystemd --name=service1
+
override_dh_makeshlibs:
for i in $(SYMBOL_LIBS); \
do \
^ permalink raw reply related [flat|nested] 7+ messages in thread* [RFC PATCH 6/2] e2scrub: skip filesystems that don't have journals
2023-12-15 22:55 [PATCHSET] e2scrub: fix some problems Darrick J. Wong
` (3 preceding siblings ...)
2024-01-10 5:57 ` [PATCH 4/2] debian: don't restart e2scrub_all when upgrading package Darrick J. Wong
@ 2024-01-10 5:58 ` Darrick J. Wong
4 siblings, 0 replies; 7+ messages in thread
From: Darrick J. Wong @ 2024-01-10 5:58 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4, Brian J. Murrell
From: Darrick J. Wong <djwong@kernel.org>
Brian J. Murrell reported that e2scrub reports failures with one of his
filesystems. From the email discussion after he supplied a metadump:
AHA! This is an ext2 filesystem, since it doesn't have the
"has_journal" or "extents" features turned on:
# e2image -r /tmp/disk.qcow2 /dev/sda
# dumpe2fs /dev/sda -h
dumpe2fs 1.47.1~WIP-2023-12-27 (27-Dec-2023)
Filesystem volume name: <none>
Last mounted on: /opt
Filesystem UUID: 2c70368a-0d54-4805-8620-fda19466d819
Filesystem magic number: 0xEF53
Filesystem revision #: 1 (dynamic)
Filesystem features: ext_attr resize_inode dir_index filetype sparse_super large_file
Filesystem flags: signed_directory_hash
Default mount options: user_xattr acl
Filesystem state: not clean with errors
(Note: Filesystem state == "clean" means that EXT2_VALID_FS is set in
the superblock s_state field; "not clean with errors" means that the
flag is not set.)
I bet the "journal only" preen doesn't actually reset the filesystem
state either:
# e2fsck -E journal_only -p /dev/sda
# dumpe2fs /dev/sda -h | grep state
dumpe2fs 1.47.1~WIP-2023-12-27 (27-Dec-2023)
Filesystem state: not clean with errors
Nope.
So now I know what happened -- when mounting an ext* filesystem that
doesn't have a journal, the driver clears EXT2_VALID_FS from the primary
superblock. This forces the system to run e2fsck after a crash, because
that's what you have to do for unjournalled filesystems.
The "e2fsck -E journal_only -p" call in e2scrub only replays the
journal. Since there is no journal, it exits almost immediately.
That's the intended behavior, but then it means that the "e2fsck -fy"
call immediately after sees that the superblock doesn't have
EXT2_VALID_FS set, sets it, and makes e2fsck return 1.
So that's why you're getting the e2scrub failures.
Contrast this to what you get when the filesystem has a journal:
# dumpe2fs -h /dev/sdb
dumpe2fs 1.47.0 (5-Feb-2023)
Filesystem volume name: <none>
Last mounted on: <not available>
Filesystem UUID: e18b8b57-a75e-4316-87ce-6a08969476c3
Filesystem magic number: 0xEF53
Filesystem revision #: 1 (dynamic)
Filesystem features: has_journal ext_attr resize_inode dir_index filetype needs_recovery sparse_super large_file
Filesystem flags: signed_directory_hash
Default mount options: user_xattr acl
Filesystem state: clean
Filesystems with journals retain their EXT4_VALID_FS state when they're
mounted.
Hmm. What e2scrub should do about unjournalled filesystems is a thorny
question. My initial thought is that it should skip them, because a
mounted unjournalled filesystem cannot by definition be kept consistent.
Therefore, teach e2scrub_all to avoid them and e2scrub to fail them at
the onset.
Restricting the scope of e2scrub sucks, but in the meantime at least it
means that your filesystem isn't massively corrupt. Thanks for the
metadump, it was very useful for root cause analysis.
Reported-by: "Brian J. Murrell" <brian@interlinx.bc.ca>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
scrub/e2scrub.in | 7 +++++++
scrub/e2scrub_all.in | 4 ++++
2 files changed, 11 insertions(+)
diff --git a/scrub/e2scrub.in b/scrub/e2scrub.in
index 7ed57f2d..043bc12b 100644
--- a/scrub/e2scrub.in
+++ b/scrub/e2scrub.in
@@ -159,6 +159,13 @@ if [ ! -e "${dev}" ]; then
exitcode 16
fi
+# Do not scrub unjournalled filesystems; they are inconsistent when mounted
+if [ "${reap}" -eq 0 ] && ! dumpe2fs -h "${dev}" | grep -q 'has_journal'; then
+ echo "${arg}: Filesystem has no journal, cannot check."
+ print_help
+ exitcode 16
+fi
+
# Make sure this is an LVM device we can snapshot
lvm_vars="$(lvs --nameprefixes -o name,vgname,lv_role --noheadings "${dev}" 2> /dev/null)"
eval "${lvm_vars}"
diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
index 437f6cc2..fe4dda95 100644
--- a/scrub/e2scrub_all.in
+++ b/scrub/e2scrub_all.in
@@ -125,6 +125,10 @@ ls_scan_targets() {
while read vars ; do
eval "${vars}"
+ # Skip unjournalled filesystems; they are inconsistent when
+ # mounted
+ dumpe2fs -h "${NAME}" | grep -q 'has_journal' || continue
+
if [ "${scrub_all}" -eq 1 ] || [ -n "${MOUNTPOINT}" ]; then
echo ${MOUNTPOINT:-${NAME}}
fi
^ permalink raw reply related [flat|nested] 7+ messages in thread