> -----Original Message----- > From: openembedded-core-boun...@lists.openembedded.org > [mailto:openembedded-core-boun...@lists.openembedded.org] On Behalf Of > Joshua Watt > Sent: den 23 maj 2017 21:57 > To: Randy Witt <randy.e.w...@linux.intel.com> > Cc: OE-core <openembedded-core@lists.openembedded.org> > Subject: Re: [OE-core] [PATCH] openssh: Atomically generate host keys > > On Tue, May 23, 2017 at 12:56 PM, Joshua Watt <jpewhac...@gmail.com> > wrote: > > On Tue, May 23, 2017 at 12:23 PM, Randy Witt > > <randy.e.w...@linux.intel.com> wrote: > >> On 05/23/2017 08:29 AM, Joshua Watt wrote: > >>> > >>> On Tue, May 23, 2017 at 9:37 AM, Burton, Ross > <ross.bur...@intel.com> > >>> wrote: > >>>> > >>>> > >>>> On 7 May 2017 at 02:33, Joshua Watt <jpewhac...@gmail.com> wrote: > >>>>> > >>>>> > >>>>> +if [ ! -f "$NAME" ]; then > >>>>> + echo " generating ssh $TYPE key..." > >>>>> + ssh-keygen -q -f "${NAME}.tmp" -N '' -t $TYPE > >>>>> + > >>>>> + # Sync to ensure data is written to temp file before > renaming > >>>>> + sync > >>>>> + > >>>>> + # Move (Atomically rename) files > >>>>> + # Rename the .pub file first, since the check that triggers > a > >>>>> + # key generation is based on the private file. > >>>>> + mv -f "${NAME}.tmp.pub" "${NAME}.pub" > >>>>> + sync > >>>>> + > >>>>> + mv -f "${NAME}.tmp" "${NAME}" > >>>>> + sync > >>>>> +fi > >>>>> > >>>> > >>>> All of these syncs seem quite enthusiastic, are they really > needed? > >>>> Writing > >>>> the file to a temporary name and then mving it to the real name > should > >>>> result in either no file or a complete file in the event of power > loss, > >>>> surely? > >>> > >>> > >>> My understanding (and observation) of most journal file systems is > >>> that only metadata (i.e. directory entries and such) are journaled > in > >>> typical usage. The first sync is necessary in this case to ensure > that > >>> the actual file data gets put on the disk before we rename the > files, > >>> otherwise in the event of interruption journaled rename might get > >>> replayed but have garbage data. The second one is more of a "force > >>> operation order" sync to make sure the public file is written > before > >>> the private one, as a reordering would cause problems. The last > sync > >>> is the most optional, but I've seen it take minutes for disk to > sync > >>> contents if no one calls "sync", so it is very possible that all > our > >>> work of regenerating keys would be for naught if power is > interrupted > >>> in the meantime. > >>> > >>> I think some of these syncs can be removed. Namely, the first and > >>> third one. The second one needs to be there, but can server double > >>> duty of syncing data to disk and enforcing the order between the > >>> public and private rename. It does mean we could get a state where > the > >>> public key exists but is garbage, but this should be OK because the > >>> private key won't exist and it would be regenerated. > >>> > >>> The third sync can be removed and I can put one final sync at the > end > >>> after all keys are generated to ensure we won't go through all the > >>> effort of regenerating the (last) key again in the event of > >>> interruption shortly after, unless you would prefer I didn't. > >>> > >> > >> The typical convention for this is, > >> > >> 1. Update file data. > >> 2. sync file > >> 3. sync containing directory > >> 4. mv file to new location > >> 5. sync directory containing new file (although I've seen this step > left out > >> before) > >> > >> https://lwn.net/Articles/457672/ is a good example which is linked > from > >> https://lwn.net/Articles/457667/ > >> > >> But one of the important parts vs your code, is also that the > example is > >> only calling sync on the files/directory needed, vs calling "sync" > with no > >> arguments which is going to cause all data in all filesystem caches > to be > >> flushed. > > > > Ah, OK. That makes sense, I will update sync to specify the > > files/directory explicitly. > > FWIW, I did some tests on the sync behavior: > > It appears that older versions of the sync command ignore any > arguments and just call sync(). From Ubuntu 14.04: > $ strace sync foo > ... > write(2, "sync: ", 6sync: ) = 6 > write(2, "ignoring all arguments", 22) = 22 > write(2, "\n", 1) = 1 > sync() = 0 > ... > > The same is true for the (default) busybox version of sync on master > (again verified with strace), but it doesn't complain nosily about it.
Busybox has a separate fsync applet to do file synchronization, but it is not enabled in the default OE-core configuration... > I looked at the coreutils code, and sync was changed to respect > arguments in January of 2015 > (8b2bf5295f353016d4f5e6a2317d55b6a8e7fd00) and only call fsync() on > the provided arguments (if any are provided). > > Either way, adding the arguments to the sync call in my patch won't > hurt because it is forward compatible, even though it won't be > optimally efficient currently because the sync command currently > simply calls sync() //Peter -- _______________________________________________ Openembedded-core mailing list Openembedded-core@lists.openembedded.org http://lists.openembedded.org/mailman/listinfo/openembedded-core
