Re: svn commit: r349256 - head/libexec/rc/rc.d
> On 21 Jun, Xin LI wrote: > > > But ultimately, I think the real design question here that needs to be > > solved would probably be "Why are piling up multiple layers of workarounds > > around 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 seems to be more appropriate for /var/run and generated at boot > > from a template located somewhere in /etc. The benefit of this approach is > > that you would have one less file to merge for each etcupdate/mergemaster > > (or at least only need to do it when some customization is made), and there > > is no need to worry about write durability. > > +1 > > This is something that has bothered me for a long time. It should be > possible to run with a read-only /etc (obviously with some functional > limitations). The updating of the kernel string in /etc/motd is what should go, that was done before the days of uname, so was more important, but now it is rather pointless to have the kernel name and version in motd. Just go back to earlier days and leave motd as it was intended, the Message of The Day from the system administrator(s). If you want readonly / you do: cd /etc mkdir ../var/etc mv motd ../var/etc ln -s ../var/etc/motd If your head hurts from the .. it is from decades of me doing chrooted stuff, sorry, just how my brain fires. You need to do the above to a fistfull of other files too, and iirc there are some issues with passwd as it unlinks the symlinks. For simpler readonly / you do: cd / mv etc var ln -s var/etc this one works very well and is what I use in my nfs diskless setups to share a readonly / as each node has its own /var file system, / and /usr are shared. /tmp varies depending on what i am doing, but most often a tmpfs. I do not use the standard freebsd diskless /var tar ball, my /var's are persistant accross reboots and are per node. -- Rod Grimes rgri...@freebsd.org ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r349256 - head/libexec/rc/rc.d
On 21 Jun, Xin LI wrote: > But ultimately, I think the real design question here that needs to be > solved would probably be "Why are piling up multiple layers of workarounds > around 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 seems to be more appropriate for /var/run and generated at boot > from a template located somewhere in /etc. The benefit of this approach is > that you would have one less file to merge for each etcupdate/mergemaster > (or at least only need to do it when some customization is made), and there > is no need to worry about write durability. +1 This is something that has bothered me for a long time. It should be possible to run with a read-only /etc (obviously with some functional limitations). ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r349256 - head/libexec/rc/rc.d
> On Jun 21, 2019, at 12:32, Xin LI wrote:
>
>
>> 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
>>
>> Log:
>> rc.d/motd: Update motd more robustly
>>
>> Use appropriate fsyncs to persist the rewritten /etc/motd file, when a
>> rewrite is performed.
>
> Why is /etc/motd so important to deserve this kind of construct? The worst
> 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 more
> problems than it intended to solve.
>
>>
>> Reported by: Jonathan Walton
>> Reviewed by: allanjude, vangyzen
>> Sponsored by: Dell EMC Isilon
>> Differential Revision:https://reviews.freebsd.org/D20701
>>
>> Modified:
>> head/libexec/rc/rc.d/motd
>>
>> Modified: head/libexec/rc/rc.d/motd
>> ==
>> --- 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 == 1) {if ($1 == "FreeBSD") {next} else {print "\n"$0}}
>> else {print}}' < /etc/motd >> ${T}
>>
>> - cmp -s $T /etc/motd || {
>> - cp $T /etc/motd
>> + if ! cmp -s $T /etc/motd; then
>> + mv -f $T /etc/.motd.tmp
>
> 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, but it
> is not safe to assume /etc/.motd.tmp is a non-existent plain file: mv would
> happily proceed when .motd.tmp is a preexisting directory, for instance (and
> subsequent mv -f would fail). A malicious system administrator can now plant
> a timing bomb which will fill / eventually.
>
> You can do a TMPDIR=/etc/ mktemp .motd.tmp. and use that to mitigate
> the attack above, but then if the system crash in the middle you would still
> end up with a dangling .motd.tmp. file in /etc.
>
> 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
> remove like before.
>
>> + fsync /etc/.motd.tmp
>> + mv -f /etc/.motd.tmp /etc/motd
>> chmod ${PERMS} /etc/motd
>> - }
>> - rm -f $T
>> + fsync /etc
>
> 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 event
> of a crash, and both are acceptable.
>
> So, in my opinion the new code would have made the situation worse than the
> old code.
>
> But ultimately, I think the real design question here that needs to be solved
> would probably be "Why are piling up multiple layers of workarounds around
> 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 seems
> to be more appropriate for /var/run and generated at boot from a template
> located somewhere in /etc. The benefit of this approach is that you would
> have one less file to merge for each etcupdate/mergemaster (or at least only
> need to do it when some customization is made), and there is no need to worry
> about write durability.
I don’t know the exact reasoning today, but once upon a time, there used to
be a number of things hooked to /etc/motd generation that could influence
information provided to downstream callers/systems.
Thank you,
-Enji
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r349256 - head/libexec/rc/rc.d
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
>
> Log:
> rc.d/motd: Update motd more robustly
>
> Use appropriate fsyncs to persist the rewritten /etc/motd file, when a
> rewrite is performed.
>
Why is /etc/motd so important to deserve this kind of construct? The worst
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
more problems than it intended to solve.
>
> Reported by: Jonathan Walton
> Reviewed by: allanjude, vangyzen
> Sponsored by: Dell EMC Isilon
> Differential Revision:https://reviews.freebsd.org/D20701
>
> Modified:
> head/libexec/rc/rc.d/motd
>
> Modified: head/libexec/rc/rc.d/motd
>
> ==
> --- 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 == 1) {if ($1 == "FreeBSD") {next} else {print
> "\n"$0}} else {print}}' < /etc/motd >> ${T}
>
> - cmp -s $T /etc/motd || {
> - cp $T /etc/motd
> + if ! cmp -s $T /etc/motd; then
> + mv -f $T /etc/.motd.tmp
>
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, but it is not safe to assume /etc/.motd.tmp is a non-existent
plain file: mv would happily proceed when .motd.tmp is a preexisting
directory, for instance (and subsequent mv -f would fail). A malicious
system administrator can now plant a timing bomb which will fill /
eventually.
You can do a TMPDIR=/etc/ mktemp .motd.tmp. and use that to
mitigate the attack above, but then if the system crash in the middle you
would still end up with a dangling .motd.tmp. file in /etc.
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
remove like before.
+ fsync /etc/.motd.tmp
>
+ mv -f /etc/.motd.tmp /etc/motd
> chmod ${PERMS} /etc/motd
> - }
> - rm -f $T
> + fsync /etc
>
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 event of a crash, and both are acceptable.
So, in my opinion the new code would have made the situation worse than the
old code.
But ultimately, I think the real design question here that needs to be
solved would probably be "Why are piling up multiple layers of workarounds
around 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 seems to be more appropriate for /var/run and generated at boot
from a template located somewhere in /etc. The benefit of this approach is
that you would have one less file to merge for each etcupdate/mergemaster
(or at least only need to do it when some customization is made), and there
is no need to worry about write durability.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r349256 - head/libexec/rc/rc.d
21.06.2019 9:37, Conrad Meyer wrote:
> Author: cem
> Date: Fri Jun 21 02:37:54 2019
> New Revision: 349256
> URL: https://svnweb.freebsd.org/changeset/base/349256
>
> Log:
> rc.d/motd: Update motd more robustly
>
> Use appropriate fsyncs to persist the rewritten /etc/motd file, when a
> rewrite is performed.
>
> Reported by:Jonathan Walton
> Reviewed by:allanjude, vangyzen
> Sponsored by: Dell EMC Isilon
> Differential Revision: https://reviews.freebsd.org/D20701
>
> Modified:
> head/libexec/rc/rc.d/motd
>
> Modified: head/libexec/rc/rc.d/motd
> ==
> --- 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 == 1) {if ($1 == "FreeBSD") {next} else {print "\n"$0}}
> else {print}}' < /etc/motd >> ${T}
>
> - cmp -s $T /etc/motd || {
> - cp $T /etc/motd
> + if ! cmp -s $T /etc/motd; then
> + mv -f $T /etc/.motd.tmp
> + fsync /etc/.motd.tmp
> + mv -f /etc/.motd.tmp /etc/motd
> chmod ${PERMS} /etc/motd
> - }
> - rm -f $T
> + fsync /etc
> + else
> + rm -f $T
> + fi
>
> check_startmsgs && echo '.'
> }
Why do we need fsync while updating small plain text file?
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
