Re: [patch] was: Re: login(3) routines data integrity patch
Chris Turner wrote: > On 11/12/15 14:10, Ted Unangst wrote: > > Chris Turner wrote: > >>> > >>> The attached patch calls fsync(2) on related FD's in the login(3) > >>> routines, which corrected the problem on my test machine, > >>> and imho might be a good idea in general. > > > > AFAIK it should not be necessary to call fsync() before close(). Closing a > > file should flush all its data. The patch either does nothing, or masks a > > much > > more serious somewhere else. (The latter is a distinct possibility, but we > > can't go adding fsync to hundreds of file operations throughout the tree.) > > Will defer - > > To be clear however, in this case I'm strictly referring to the (brief) > time window between data being flushed from the process and that data then > being sync'ed to disk by the system - > > E.g., as related to fsync(2): > > " > fsync() and fdatasync() should be used by programs that require a file > to > be in a known state, for example, in building a simple transaction > facility. > " > > obviously there's still the case of physical disk caches, etc. > > I realize fsync is not done everywhere on file close, and likely rightfully > so in many/most cases - my thinking was that here specifically might be a > good place given the type of (sensitive, non-reproducable) data being stored. No, you're not wrong. The question here is what is the "transaction" that needs to be in a known state. Anything reading this file will see the cached data, so no inconsistency. also afaik, there aren't any other files which depend on this file, but which may be written out first. there's no requirement for this data to be written out in a particular order with respect to other data. The window in which the data is not written you refer to also exists before (and even during) the fsync call. That data should also be written cleanly before shutdown. fwiw other people have seen other symptoms of the system failing to fully flush write buffers before shutdown. some aspect of vfs_sync/shutdown is not trying "hard enough" to get the job done.
Re: [patch] was: Re: login(3) routines data integrity patch
On 11/12/15 14:10, Ted Unangst wrote: Chris Turner wrote: >>> The attached patch calls fsync(2) on related FD's in the login(3) routines, which corrected the problem on my test machine, and imho might be a good idea in general. AFAIK it should not be necessary to call fsync() before close(). Closing a file should flush all its data. The patch either does nothing, or masks a much more serious somewhere else. (The latter is a distinct possibility, but we can't go adding fsync to hundreds of file operations throughout the tree.) Will defer - To be clear however, in this case I'm strictly referring to the (brief) time window between data being flushed from the process and that data then being sync'ed to disk by the system - E.g., as related to fsync(2): " fsync() and fdatasync() should be used by programs that require a file to be in a known state, for example, in building a simple transaction facility. " obviously there's still the case of physical disk caches, etc. I realize fsync is not done everywhere on file close, and likely rightfully so in many/most cases - my thinking was that here specifically might be a good place given the type of (sensitive, non-reproducable) data being stored. Again, deferring, but, thought i'd clarify a bit more of my thinking in case it might be beneficial to the discussion. Cheers, - Chris
Re: [patch] was: Re: login(3) routines data integrity patch
Chris Turner wrote: > > Wondering if anyone had a chance to take a look at these - > Subject line tagged accordingly :D > > I could see in some scenarios, aside from generating incorrect > > data, this incorrect record could be used to facillitate hiding > > presence of a successful compromise. > > > > The attached patch calls fsync(2) on related FD's in the login(3) > > routines, which corrected the problem on my test machine, > > and imho might be a good idea in general. AFAIK it should not be necessary to call fsync() before close(). Closing a file should flush all its data. The patch either does nothing, or masks a much more serious somewhere else. (The latter is a distinct possibility, but we can't go adding fsync to hundreds of file operations throughout the tree.)
[patch] was: Re: login(3) routines data integrity patch
Wondering if anyone had a chance to take a look at these - Subject line tagged accordingly :D Cheers, - Chris On 10/30/15 11:44, Chris Turner wrote: Hello - I was testing some login data collection scripts (on a VM) and discovered that in certain cases, it was possible for a login record to not be fully commited to disk prior to system shutdown, resulting in the last(1) entry for the login not being visible. (was doing e.g. ssh root@testbox to generate wtmp login records and then powering off the vm to see if my code processed unclean shutdown records correctly). I could see in some scenarios, aside from generating incorrect data, this incorrect record could be used to facillitate hiding presence of a successful compromise. The attached patch calls fsync(2) on related FD's in the login(3) routines, which corrected the problem on my test machine, and imho might be a good idea in general. The patch was generated on 5.8 current, but based on a rudimentary check of head it looks like it should apply cleanly and is 3 lines of diff if not :) It might be useful also to have last(1) warn of truncated/ incomplete records as were the case in my (I think now lost) corrupt wtmp file.. I did not attempt to implement this. Please let me know if there are any questions or concerns and thanks for a great system. Thanks, - Chris
