[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: pam_unix doesn't fsync the tmp passwd file before overwriting /etc/passwd



On Tue, Mar 17, 2009 at 07:44:34AM +0100, Ermanno Scaglione wrote:
> Recenty there is much rumor in Internet because of some cases of data
> loss caused by the filesystem ext4 in case of a crash. Theodore T'so who
> wrote the filesystem maintains that is is mainly responsibility of
> application and system programmers that do not call properly fsync on
> the files.

Yes, that's his opinion as it relates to application vs. filesystem
responsibilities in general, however he has nevertheless already
included a workaround to restore the "traditional" semantics
specifically for ext4:

http://git.kernel.org/?p=linux/kernel/git/tytso/ext4.git;a=commitdiff;h=dbc85aa9f11d8c13c15527d43a3def8d7beffdc8

The continuing argument is thus about whether applications need to be
"fixed" to call fsync(), or some new fsync-like call should be invented
to be used in such cases, or other new filesystems must adhere to the
"traditional" semantics (or be considered broken if they don't).  It is
no longer about ext4 specifically, which has the workaround (and Ted
does not appear to have an intent of dropping that workaround, which is
great).

I put the words "traditional" and "fixed" in quotes, because both are
being debated (by different people).

[ My own opinion is that regardless of whether the "traditional"
semantics have in fact been followed by all common/historical Unix
filesystems or not, they should be made the standard now, because they
are nice to have.  So the ext4 workaround was the right thing to do, and
more needs to be done (other new filesystems asked to do the same).  If
this is acknowledged as a standard, then no extra fsync-like syscall is
needed.  And mandating that all programs fsync() before rename(), or do
this in an async thread (which does not fully avoid non-optimal disk
I/O), is a worse thing to do, because the result is worse.  But that's
just my opinion, which I mention because I am posting a message anyway;
I am perfectly aware that lots of people will disagree, and this is
definitely not to be discussed on pam-list.  In the above comments,
outside of these brackets, I tried to stay neutral - and those comments
are what makes this posting suitable for pam-list. ]

As to (not) introducing fsync() calls into pam_unix specifically, I
don't have strong feelings for or against this, although it is OK to do
in order to be extra-safe on some modern filesystems other than ext4.

I include some more context below:

> Since some bug report on Ubuntu say that the file /etc/passwd
> and /etc/shadow where lost (0 length) because the computer crashed just
> after changing a password I decided to give a look to the sources to see
> if T'so was right and in fact in modules/pam_unix/passverify.c fsync is
> never called before closing the file. A small patch like the one
> appended certainly will not hurt and it is more correct formally. Always
> more systems are using delayed allocation and the problem will became
> more common. 
> 
> 
> -https://bugs.edge.launchpad.net/ubuntu/+source/linux/+bug/317781/comments/54
> -http://thunk.org/tytso/blog/2009/03/12/delayed-allocation-and-the-zero-length-file-problem/
> -http://thunk.org/tytso/blog/2009/03/15/dont-fear-the-fsync/
> 
> 
> diff -r -u Linux-PAM-1.0.4/modules/pam_unix/passverify.c
> Linux-PAM-1.0.4.new/modules/pam_unix/passverify.c
> --- Linux-PAM-1.0.4/modules/pam_unix/passverify.c	2009-03-02
> 16:02:22.000000000 +0100
> +++ Linux-PAM-1.0.4.new/modules/pam_unix/passverify.c	2009-03-16
> 22:25:20.794367897 +0100
> @@ -675,11 +675,10 @@
>  	}
>      }
>  
> -    if (fclose(pwfile)) {
> +    if (fsync(pwfile)||fclose(pwfile)) {
>  	D(("error writing entries to old passwords file: %m"));
>  	err = 1;
>      }
> -
>  done:
>      if (!err) {
>  	if (rename(OPW_TMPFILE, OLD_PASSWORDS_FILE))
> @@ -795,7 +794,7 @@
>      }
>      fclose(opwfile);
>  
> -    if (fclose(pwfile)) {
> +    if (fsync(pwfile)||fclose(pwfile)) {
>  	D(("error writing entries to password file: %m"));
>  	err = 1;
>      }
> @@ -925,7 +924,7 @@
>      }
>      fclose(opwfile);
>  
> -    if (fclose(pwfile)) {
> +    if (fsync(pwfile)||fclose(pwfile)) {
>  	D(("error writing entries to shadow file: %m"));
>  	err = 1;
>      }

-- 
Alexander Peslyak <solar at openwall.com>
GPG key ID: 5B341F15  fp: B3FB 63F4 D7A3 BCCC 6F6E  FC55 A2FC 027C 5B34 1F15
http://www.openwall.com - bringing security into open computing environments


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]