Date: Fri, 21 Jun 2019 12:55:17 -0700 From: Enji Cooper <yaneurabeya@gmail.com> To: Xin LI <delphij@gmail.com> Cc: Conrad Meyer <cem@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org> Subject: Re: svn commit: r349256 - head/libexec/rc/rc.d Message-ID: <DA0C3941-1D53-4D9E-9B15-13B38A20D9A1@gmail.com> In-Reply-To: <CAGMYy3shArmDCEkwA_QH3Yf7FFciuJ%2Bpvd%2BX10s_Jn%2BSmziVvQ@mail.gmail.com> References: <201906210237.x5L2bt8I012721@repo.freebsd.org> <CAGMYy3shArmDCEkwA_QH3Yf7FFciuJ%2Bpvd%2BX10s_Jn%2BSmziVvQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Jun 21, 2019, at 12:32, Xin LI <delphij@gmail.com> wrote: >=20 >=20 >> On Thu, Jun 20, 2019 at 7:38 PM Conrad Meyer <cem@freebsd.org> 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 <jonathan AT isilon.com> >> 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=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?DA0C3941-1D53-4D9E-9B15-13B38A20D9A1>