linux-ext4 - Re: [PATCH 3/7] e2scrub: add service (cron, systemd) support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <C49C8EAD-E9B8-4F66-9938-F6A2638C11AE@dilger.ca>
Date:   Thu, 1 Mar 2018 16:04:46 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     Theodore Ts'o <tytso@....edu>, djwong@...nel.org,
        linux-ext4@...r.kernel.org
Subject: Re: [PATCH 3/7] e2scrub: add service (cron, systemd) support

On Mar 1, 2018, at 11:23 AM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> 
> From: Darrick J. Wong <darrick.wong@...cle.com>
> 
> Add the ability to run the e2scrub utilities as a periodically scheduled
> system service.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@...cle.com>
> ---
> diff --git a/configure.ac b/configure.ac
> index 6ffff5d..34935b7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1391,7 +1391,8 @@ else
> 
> dnl
> +dnl Where do cron jobs go?
> +dnl
> +AC_ARG_WITH([crond_dir],
> +  [AS_HELP_STRING([--with-crond-dir@<:@=DIR@:>@],
> +	[Install system crontabs into DIR.])],
> +  [],
> +  [with_crond_dir=yes])
> +AS_IF([test "x${with_crond_dir}" != "xno"],
> +  [
> +	AS_IF([test "x${with_crond_dir}" = "xyes"],
> +	  [
> +		AS_IF([test -d "/etc/cron.d"],
> +		  [with_crond_dir="/etc/cron.d"])
> +	  ])
> +	AC_MSG_CHECKING([for system crontab dir])
> +	crond_dir="${with_crond_dir}"
> +	AS_IF([test -n "${crond_dir}"],
> +	  [
> +		AC_MSG_RESULT(${crond_dir})
> +		have_crond="yes"
> +	  ],
> +	  [
> +		AC_MSG_RESULT(no)
> +		have_crond="no"
> +	  ])
> +  ],
> +  [
> +	have_crond="disabled"
> +  ])
> +AC_SUBST(have_crond)
> +AC_SUBST(crond_dir)
> 
> 
> diff --git a/scrub/e2scrub_all.cron.in b/scrub/e2scrub_all.cron.in
> new file mode 100644
> index 0000000..0c133bd
> --- /dev/null
> +++ b/scrub/e2scrub_all.cron.in
> @@ -0,0 +1,2 @@
> +30 3 * * 0 root test -e /run/systemd/system || @root_sbindir@...scrub_all
> +10 3 * * * root test -e /run/systemd/system || @libdir@...scrub_reap
> diff --git a/scrub/e2scrub_all.in b/scrub/e2scrub_all.in
> index ff9eb8f..5da9a42 100644
> --- a/scrub/e2scrub_all.in
> +++ b/scrub/e2scrub_all.in
> @@ -24,6 +24,22 @@ types="ext2,ext3,ext4"
> exitcode() {
> 	ret="$1"
> 
> +	# If we're being run as a service, the return code must fit the LSB
> +	# init script action error guidelines, which is to say that we
> +	# compress all errors to 1 ("generic or unspecified error", LSB 5.0
> +	# section 22.2) and hope the admin will scan the log for what
> +	# actually happened.
> +
> +	# We have to sleep 2 seconds here because journald uses the pid to
> +	# connect our log messages to the systemd service.  This is critical
> +	# for capturing all the log messages if the scrub fails, because the
> +	# fail service uses the service name to gather log messages for the
> +	# error report.
> +	if [ -n "${SERVICE_MODE}" ]; then
> +		test "${ret}" -ne 0 && ret=1

It isn't clear that this is doing what you expect?  "test N" returns 0 for
all values of N, so this is just making ret=1 always.  It would be more clear
to use:

		[ $ret -ne 0 ] && ret=1


> +		test -x "${SLEEP_PROG}" && "${SLEEP_PROG}" 2

Why even depend on an external sleep?  You can just use the shell built-in,
if it is available.

> +	fi
> +
> 	exit "${ret}"
> }
> 
> @@ -39,6 +55,8 @@ prog_path() {
> 
> LVS_PROG="$(prog_path "@root_sbindir@...s" "lvs")"
> BLKID_PROG="$(prog_path "@root_sbindir@...kid" "blkid")"
> +SYSTEMCTL_PROG="$(prog_path "@root_bindir@...stemctl")"
> +SLEEP_PROG="$(prog_path "@root_bindir@...eep")"

More obfuscation that should be removed, IMHO.

> # Scrub any fs on lvm by creating a snapshot and fscking that.
> "${LVS_PROG}" -o vg_name,lv_name,lv_role --noheadings 2> /dev/null | while read vg lv role extra; do
> @@ -51,7 +69,15 @@ BLKID_PROG="$(prog_path "@root_sbindir@...kid" "blkid")"
> 	# Skip non-ext[234]
> 	"${BLKID_PROG}" -p -n "${types}" "${dev}" > /dev/null 2>&1 || continue
> 
> -	${DBG} "@root_sbindir@...scrub" "${dev}"
> +	if [ ! -x "${SYSTEMCTL_PROG}" ]; then
> +		${DBG} "@root_sbindir@...scrub" "${dev}"
> +	else
> +		${DBG} "${SYSTEMCTL_PROG}" start "e2scrub@...ev}" 2> /dev/null
> +		res=$?
> +		if [ "${res}" -ne 0 ] && [ "${res}" -ne 1 ]; then
> +			${DBG} "@root_sbindir@...scrub" "${dev}"
> +		fi
> +	fi
> done
> 
> exitcode 0
> diff --git a/scrub/e2scrub_fail.in b/scrub/e2scrub_fail.in
> new file mode 100644
> index 0000000..c1696a0
> --- /dev/null
> +++ b/scrub/e2scrub_fail.in
> @@ -0,0 +1,26 @@
> +#!/bin/bash
> +
> +# Email logs of failed e2scrub unit runs
> +
> +mailer=/usr/sbin/sendmail

It would be better not to hard-code the mailer path.

> +(cat << ENDL
> +To: $1
> +From: <e2scrub@...ostname}>
> +Subject: e2scrub failure on ${device}
> +
> +So sorry, the automatic e2scrub of ${device} on ${hostname} failed.
> +
> +A log of what happened follows:
> +ENDL
> +systemctl status --full --lines 4294967295 "e2scrub@...evice}") | "${mailer}" -t -i

This shouldn't depend on systemd to generate the error log.  Why not just
save the log in /tmp and then send it?  That would also work for cron on
non-systemd systems.  Or does the e2scrub command just print to stdout/stderr
and that is captured and mailed by cron directly?


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)