From owner-svn-src-head@freebsd.org Fri Jun 21 19:55:21 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1604B15BD531; Fri, 21 Jun 2019 19:55:21 +0000 (UTC) (envelope-from yaneurabeya@gmail.com) Received: from mail-pf1-x444.google.com (mail-pf1-x444.google.com [IPv6:2607:f8b0:4864:20::444]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 9008E76394; Fri, 21 Jun 2019 19:55:20 +0000 (UTC) (envelope-from yaneurabeya@gmail.com) Received: by mail-pf1-x444.google.com with SMTP id m30so4107627pff.8; Fri, 21 Jun 2019 12:55:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=npe6X+w1uPCjmmoJrRTZkikXeuQUwdJkCFqxhAjRX5g=; b=sTVCXuP8i5tqKaUjmVNzersyr7UcXFpYlCVNC1W4GxYlWsjvoqqS0PCGhvI95PKJHV bQRzDOrMZWReorkwQSrXeQwDvXt/qJyK0nUDMA6MchBAT+xrlGSEtfEAPozdmOvnlzeT 1qVsbQIGbnYeZwpd1LkWYAsMaFDj7szwFGU+xC7r5MSlfYkdTjQCcky5juncGnrRI+nA iqhB0AxCgdgNQbP6/AC+/VQvf4g5ejs0KKVNWkh0yQ6Mwqd1TZeMyzU3yVf4P4bYgxL/ NupfsIv1j4ZVIOVOt/bEROPsVB1LERYwFzbXQvEz414MdDNsSirE55XaFeiP63RX+/Ch PNSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=npe6X+w1uPCjmmoJrRTZkikXeuQUwdJkCFqxhAjRX5g=; b=BvPCVcUP3l7pK4EsSbzGBlRYiOePifQwwpAoqYjdNkOHuY9BVtX0F2qUkiJVJpDd1J /6BjjNsCjArXqlPOj18+3mvK63L8dcp6JOnaVK2a/9dlE1adupYumTvgcRjP6J9StofC vwKy+uPLTBWjvBn1YBAPmugVhO9ba9bkYWGdD0e+2lPCVli8H5j6MvjbUscMZw4jAwZ+ 3WDSn0hFe6jzAH/bqCnaSxfynFrQs9SDbmnEKId2eljkUUaVLHT18h2uOnsQPIw1xzMa UM8qbMduUu2Rqifj0p20dKJjuuOiSFfDoWHNQNSgNuaSAIX6TDdACBrIcFVSf62X77tu YZ3A== X-Gm-Message-State: APjAAAWLMycPwNWaZ6l7N0K3jkIebf5pQdroNJDCEdtZ3Zql5PS+4VSj 0/qbs3NvauOPmggcvodj1OR/bOwf X-Google-Smtp-Source: APXvYqxgZboDZz/jFN+NcypFCb9gJLO2gvUsTiR9fFjCd/66UmQYBQd2R4QBZt87tCO84doCRo4wNQ== X-Received: by 2002:a17:90a:30cf:: with SMTP id h73mr9028253pjb.42.1561146919311; Fri, 21 Jun 2019 12:55:19 -0700 (PDT) Received: from ?IPv6:2607:fb90:8364:6ea7:c064:9286:67ab:e20a? ([2607:fb90:8364:6ea7:c064:9286:67ab:e20a]) by smtp.gmail.com with ESMTPSA id g5sm4165134pfm.54.2019.06.21.12.55.18 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 21 Jun 2019 12:55:18 -0700 (PDT) Mime-Version: 1.0 (1.0) Subject: Re: svn commit: r349256 - head/libexec/rc/rc.d From: Enji Cooper X-Mailer: iPhone Mail (16F203) In-Reply-To: Date: Fri, 21 Jun 2019 12:55:17 -0700 Cc: Conrad Meyer , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Message-Id: References: <201906210237.x5L2bt8I012721@repo.freebsd.org> To: Xin LI X-Rspamd-Queue-Id: 9008E76394 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; NEURAL_HAM_SHORT(-0.98)[-0.982,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Jun 2019 19:55:21 -0000 > On Jun 21, 2019, at 12:32, Xin LI wrote: >=20 >=20 >> On Thu, Jun 20, 2019 at 7:38 PM Conrad Meyer wrote: >> Author: cem >> Date: Fri Jun 21 02:37:54 2019 >> New Revision: 349256 >> URL: https://svnweb.freebsd.org/changeset/base/349256 >>=20 >> Log: >> rc.d/motd: Update motd more robustly >>=20 >> Use appropriate fsyncs to persist the rewritten /etc/motd file, when a >> rewrite is performed. >=20 > Why is /etc/motd so important to deserve this kind of construct? The wors= t that could happen to /etc/motd with previous code is that you might end up= with an empty or non-existent /etc/motd, and the change have introduced mor= e problems than it intended to solve. > =20 >>=20 >> Reported by: Jonathan Walton >> Reviewed by: allanjude, vangyzen >> Sponsored by: Dell EMC Isilon >> Differential Revision: https://reviews.freebsd.org/D20701 >>=20 >> Modified: >> head/libexec/rc/rc.d/motd >>=20 >> Modified: head/libexec/rc/rc.d/motd >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D >> --- head/libexec/rc/rc.d/motd Fri Jun 21 00:52:30 2019 (r349255)= >> +++ head/libexec/rc/rc.d/motd Fri Jun 21 02:37:54 2019 (r349256)= >> @@ -37,11 +37,15 @@ motd_start() >> uname -v | sed -e 's,^\([^#]*\) #\(.* [1-2][0-9][0-9][0-9]\).*/\(= [^\]*\) $,\1 (\3) #\2,' > ${T} >> awk '{if (NR =3D=3D 1) {if ($1 =3D=3D "FreeBSD") {next} else {pri= nt "\n"$0}} else {print}}' < /etc/motd >> ${T} >>=20 >> - cmp -s $T /etc/motd || { >> - cp $T /etc/motd >> + if ! cmp -s $T /etc/motd; then >> + mv -f $T /etc/.motd.tmp >=20 > This have partially defeated the benefit of doing mktemp in the code above= . The old code tries to avoid excessive writes to /, which is preserved, bu= t it is not safe to assume /etc/.motd.tmp is a non-existent plain file: mv w= ould happily proceed when .motd.tmp is a preexisting directory, for instance= (and subsequent mv -f would fail). A malicious system administrator can no= w plant a timing bomb which will fill / eventually. >=20 > You can do a TMPDIR=3D/etc/ mktemp .motd.tmp.XXXXXXXX and use that to miti= gate the attack above, but then if the system crash in the middle you would s= till end up with a dangling .motd.tmp.XXXXXXXX file in /etc. >=20 > Also note that $T is on /tmp which is likely to be a different filesystem,= internally this is mostly equivalent to cp + rm. You can simplify the code= by reverting to previous cp and keep the rm -f below as an unconditional re= move like before. >=20 >> + fsync /etc/.motd.tmp >> + mv -f /etc/.motd.tmp /etc/motd >> chmod ${PERMS} /etc/motd >> - } >> - rm -f $T >> + fsync /etc >=20 > There is absolutely no reason to fsync /etc here. mv'ing on the same file= system is performed with rename(2) which is required to be atomic by POSIX.= /etc/motd will be pointing to either the old one or the new one in the eve= nt of a crash, and both are acceptable. > =20 > So, in my opinion the new code would have made the situation worse than th= e old code. >=20 > But ultimately, I think the real design question here that needs to be sol= ved would probably be "Why are piling up multiple layers of workarounds arou= nd motd? Does it even need to be located in /etc?" The contents is meant to= be updated every time when there is a kernel change, and to that extent it s= eems to be more appropriate for /var/run and generated at boot from a templa= te located somewhere in /etc. The benefit of this approach is that you woul= d have one less file to merge for each etcupdate/mergemaster (or at least on= ly need to do it when some customization is made), and there is no need to w= orry about write durability. I don=E2=80=99t know the exact reasoning today, but once upon a time, th= ere used to be a number of things hooked to /etc/motd generation that could i= nfluence information provided to downstream callers/systems. Thank you, -Enji=