Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb

[Alan Somers]

On Thu, Oct 13, 2016 at 5:06 PM, Don Lewis  wrote:
> On 13 Oct, Alan Somers wrote:
>> On Thu, Jul 2, 2015 at 11:31 AM, Renato Botelho  wrote:
>>> Author: garga (ports committer)
>>> Date: Thu Jul  2 17:30:59 2015
>>> New Revision: 285050
>>> URL: https://svnweb.freebsd.org/changeset/base/285050
>>>
>>> Log:
>>>   When passwd or group information is changed (by pw, vipw, chpass, ...)
>>>   temporary file is created and then a rename() call move it to official 
>>> file.
>>>   This operation didn't have any check to make sure data was written to disk
>>>   and if a power cycle happens system could end up with a 0 length passwd
>>>   or group database.
>>>
>>>   There is a pfSense bug with more infor about it:
>>>
>>>   https://redmine.pfsense.org/issues/4523
>>>
>>>   The following changes were made to protect passwd and group operations:
>>>
>>>   * lib/libutil/gr_util.c:
>>>- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
>>>- After rename(), fsync() call on directory for faster result
>>>
>>>   * lib/libutil/pw_util.c
>>>- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
>>>
>>>   * usr.sbin/pwd_mkdb/pwd_mkdb.c
>>>- Added O_SYNC flag on dbopen() calls
>>>- After rename(), fsync() call on directory for faster result
>>>
>>>   * lib/libutil/pw_util.3
>>>- pw_lock() returns a file descriptor to master password file on success
>>>
>>>   Differential Revision:https://reviews.freebsd.org/D2978
>>>   Approved by:  bapt
>>>   Sponsored by: Netgate
>>>
>>> Modified:
>>>   head/lib/libutil/gr_util.c
>>>   head/lib/libutil/pw_util.3
>>>   head/lib/libutil/pw_util.c
>>>   head/usr.sbin/pwd_mkdb/pwd_mkdb.c
>>
>> This change is making certain pw operations very slow on ZFS root
>> systems.  The problem is that when you open a file with O_SYNC, every
>> single write(2) call turns into a zil_commit on ZFS, which is fairly
>> expensive.  Did you consider using fsync(2) on the temporary files
>> instead of opening them with O_SYNC?  I just tried that now, and I see
>> a considerable speedup when running the tests in
>> /usr/tests/usr.sbin/pw:
>>
>> Using O_SYNC, as CURRENT does: 4 minutes 5.2 seconds
>> No synchronous operations at all: 49.5 seconds
>> Using fsync(2): 56.0 seconds
>
> pwd_mkdb was fixed back in February with by switching to fsync() in
> r295925.  It looks like libutil was not fixed, though.

https://reviews.freebsd.org/D8319
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb

[Don Lewis]

On 13 Oct, Alan Somers wrote:
> On Thu, Jul 2, 2015 at 11:31 AM, Renato Botelho  wrote:
>> Author: garga (ports committer)
>> Date: Thu Jul  2 17:30:59 2015
>> New Revision: 285050
>> URL: https://svnweb.freebsd.org/changeset/base/285050
>>
>> Log:
>>   When passwd or group information is changed (by pw, vipw, chpass, ...)
>>   temporary file is created and then a rename() call move it to official 
>> file.
>>   This operation didn't have any check to make sure data was written to disk
>>   and if a power cycle happens system could end up with a 0 length passwd
>>   or group database.
>>
>>   There is a pfSense bug with more infor about it:
>>
>>   https://redmine.pfsense.org/issues/4523
>>
>>   The following changes were made to protect passwd and group operations:
>>
>>   * lib/libutil/gr_util.c:
>>- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
>>- After rename(), fsync() call on directory for faster result
>>
>>   * lib/libutil/pw_util.c
>>- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
>>
>>   * usr.sbin/pwd_mkdb/pwd_mkdb.c
>>- Added O_SYNC flag on dbopen() calls
>>- After rename(), fsync() call on directory for faster result
>>
>>   * lib/libutil/pw_util.3
>>- pw_lock() returns a file descriptor to master password file on success
>>
>>   Differential Revision:https://reviews.freebsd.org/D2978
>>   Approved by:  bapt
>>   Sponsored by: Netgate
>>
>> Modified:
>>   head/lib/libutil/gr_util.c
>>   head/lib/libutil/pw_util.3
>>   head/lib/libutil/pw_util.c
>>   head/usr.sbin/pwd_mkdb/pwd_mkdb.c
> 
> This change is making certain pw operations very slow on ZFS root
> systems.  The problem is that when you open a file with O_SYNC, every
> single write(2) call turns into a zil_commit on ZFS, which is fairly
> expensive.  Did you consider using fsync(2) on the temporary files
> instead of opening them with O_SYNC?  I just tried that now, and I see
> a considerable speedup when running the tests in
> /usr/tests/usr.sbin/pw:
> 
> Using O_SYNC, as CURRENT does: 4 minutes 5.2 seconds
> No synchronous operations at all: 49.5 seconds
> Using fsync(2): 56.0 seconds

pwd_mkdb was fixed back in February with by switching to fsync() in
r295925.  It looks like libutil was not fixed, though.


___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb

[Alan Somers]

On Thu, Jul 2, 2015 at 11:31 AM, Renato Botelho  wrote:
> Author: garga (ports committer)
> Date: Thu Jul  2 17:30:59 2015
> New Revision: 285050
> URL: https://svnweb.freebsd.org/changeset/base/285050
>
> Log:
>   When passwd or group information is changed (by pw, vipw, chpass, ...)
>   temporary file is created and then a rename() call move it to official file.
>   This operation didn't have any check to make sure data was written to disk
>   and if a power cycle happens system could end up with a 0 length passwd
>   or group database.
>
>   There is a pfSense bug with more infor about it:
>
>   https://redmine.pfsense.org/issues/4523
>
>   The following changes were made to protect passwd and group operations:
>
>   * lib/libutil/gr_util.c:
>- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
>- After rename(), fsync() call on directory for faster result
>
>   * lib/libutil/pw_util.c
>- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
>
>   * usr.sbin/pwd_mkdb/pwd_mkdb.c
>- Added O_SYNC flag on dbopen() calls
>- After rename(), fsync() call on directory for faster result
>
>   * lib/libutil/pw_util.3
>- pw_lock() returns a file descriptor to master password file on success
>
>   Differential Revision:https://reviews.freebsd.org/D2978
>   Approved by:  bapt
>   Sponsored by: Netgate
>
> Modified:
>   head/lib/libutil/gr_util.c
>   head/lib/libutil/pw_util.3
>   head/lib/libutil/pw_util.c
>   head/usr.sbin/pwd_mkdb/pwd_mkdb.c

This change is making certain pw operations very slow on ZFS root
systems.  The problem is that when you open a file with O_SYNC, every
single write(2) call turns into a zil_commit on ZFS, which is fairly
expensive.  Did you consider using fsync(2) on the temporary files
instead of opening them with O_SYNC?  I just tried that now, and I see
a considerable speedup when running the tests in
/usr/tests/usr.sbin/pw:

Using O_SYNC, as CURRENT does: 4 minutes 5.2 seconds
No synchronous operations at all: 49.5 seconds
Using fsync(2): 56.0 seconds

-Alan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb

[Ermal Luçi]

On Thu, Jul 2, 2015 at 7:31 PM, Renato Botelho ga...@freebsd.org wrote:

 Author: garga (ports committer)
 Date: Thu Jul  2 17:30:59 2015
 New Revision: 285050
 URL: https://svnweb.freebsd.org/changeset/base/285050

 Log:
   When passwd or group information is changed (by pw, vipw, chpass, ...)
   temporary file is created and then a rename() call move it to official
 file.
   This operation didn't have any check to make sure data was written to
 disk
   and if a power cycle happens system could end up with a 0 length passwd
   or group database.

   There is a pfSense bug with more infor about it:

   https://redmine.pfsense.org/issues/4523

   The following changes were made to protect passwd and group operations:

   * lib/libutil/gr_util.c:
- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
- After rename(), fsync() call on directory for faster result

   * lib/libutil/pw_util.c
- Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file

   * usr.sbin/pwd_mkdb/pwd_mkdb.c
- Added O_SYNC flag on dbopen() calls
- After rename(), fsync() call on directory for faster result

   * lib/libutil/pw_util.3
- pw_lock() returns a file descriptor to master password file on success

   Differential Revision:https://reviews.freebsd.org/D2978
   Approved by:  bapt
   Sponsored by: Netgate

 Modified:
   head/lib/libutil/gr_util.c
   head/lib/libutil/pw_util.3
   head/lib/libutil/pw_util.c
   head/usr.sbin/pwd_mkdb/pwd_mkdb.c

 Modified: head/lib/libutil/gr_util.c

 ==
 --- head/lib/libutil/gr_util.c  Thu Jul  2 16:17:05 2015(r285049)
 +++ head/lib/libutil/gr_util.c  Thu Jul  2 17:30:59 2015(r285050)
 @@ -141,7 +141,7 @@ gr_tmp(int mfd)
 errno = ENAMETOOLONG;
 return (-1);
 }
 -   if ((tfd = mkstemp(tempname)) == -1)
 +   if ((tfd = mkostemp(tempname, O_SYNC)) == -1)
 return (-1);
 if (mfd != -1) {
 while ((nr = read(mfd, buf, sizeof(buf)))  0)
 @@ -318,10 +318,28 @@ gr_copy(int ffd, int tfd, const struct g
  int
  gr_mkdb(void)
  {
 +   int fd;
 +
 if (chmod(tempname, 0644) != 0)
 return (-1);

 -   return (rename(tempname, group_file));
 +   if (rename(tempname, group_file) != 0)
 +   return (-1);
 +
 +   /*
 +* Make sure new group file is safe on disk. To improve
 performance we
 +* will call fsync() to the directory where file lies
 +*/
 +   if ((fd = open(group_dir, O_RDONLY|O_DIRECTORY)) == -1)
 +   return (-1);
 +


This really is not a real failure!
Not sure how you would report this but it really is not a failure since the
rename has completed and you are giving false information back.



 +   if (fsync(fd) != 0) {
 +   close(fd);
 +   return (-1);
 +   }
 +
 +   close(fd);
 +   return(0);
  }

  /*

 Modified: head/lib/libutil/pw_util.3

 ==
 --- head/lib/libutil/pw_util.3  Thu Jul  2 16:17:05 2015(r285049)
 +++ head/lib/libutil/pw_util.3  Thu Jul  2 17:30:59 2015(r285050)
 @@ -233,7 +233,8 @@ function returns 0 in case of success an
  The
  .Fn pw_lock
  function locks the master password file.
 -It returns 0 in case of success and -1 in case of failure.
 +It returns a file descriptor to master password file in case of success
 +and -1 in case of failure.
  .Pp
  The
  .Fn pw_scan

 Modified: head/lib/libutil/pw_util.c

 ==
 --- head/lib/libutil/pw_util.c  Thu Jul  2 16:17:05 2015(r285049)
 +++ head/lib/libutil/pw_util.c  Thu Jul  2 17:30:59 2015(r285050)
 @@ -226,7 +226,7 @@ pw_tmp(int mfd)
 errno = ENAMETOOLONG;
 return (-1);
 }
 -   if ((tfd = mkstemp(tempname)) == -1)
 +   if ((tfd = mkostemp(tempname, O_SYNC)) == -1)
 return (-1);
 if (mfd != -1) {
 while ((nr = read(mfd, buf, sizeof(buf)))  0)

 Modified: head/usr.sbin/pwd_mkdb/pwd_mkdb.c

 ==
 --- head/usr.sbin/pwd_mkdb/pwd_mkdb.c   Thu Jul  2 16:17:05 2015
 (r285049)
 +++ head/usr.sbin/pwd_mkdb/pwd_mkdb.c   Thu Jul  2 17:30:59 2015
 (r285050)
 @@ -51,6 +51,7 @@ __FBSDID($FreeBSD$);
  #include err.h
  #include errno.h
  #include fcntl.h
 +#include libgen.h
  #include limits.h
  #include pwd.h
  #include signal.h
 @@ -227,14 +228,14 @@ main(int argc, char *argv[])
 clean = FILE_INSECURE;
 cp(buf2, buf, PERM_INSECURE);
 dp = dbopen(buf,
 -   O_RDWR|O_EXCL, PERM_INSECURE, DB_HASH, openinfo);
 +   O_RDWR|O_EXCL|O_SYNC, PERM_INSECURE, DB_HASH,
 openinfo);

Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb

[Ian Lepore]

On Thu, 2015-07-02 at 20:00 +0200, Ermal Luçi wrote:
 On Thu, Jul 2, 2015 at 7:31 PM, Renato Botelho ga...@freebsd.org wrote:
 
  Author: garga (ports committer)
  Date: Thu Jul  2 17:30:59 2015
  New Revision: 285050
  URL: https://svnweb.freebsd.org/changeset/base/285050
 
  Log:
When passwd or group information is changed (by pw, vipw, chpass, ...)
temporary file is created and then a rename() call move it to official
  file.
This operation didn't have any check to make sure data was written to
  disk
and if a power cycle happens system could end up with a 0 length passwd
or group database.
 
There is a pfSense bug with more infor about it:
 
https://redmine.pfsense.org/issues/4523
 
The following changes were made to protect passwd and group operations:
 
* lib/libutil/gr_util.c:
 - Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
 - After rename(), fsync() call on directory for faster result
 
* lib/libutil/pw_util.c
 - Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
 
* usr.sbin/pwd_mkdb/pwd_mkdb.c
 - Added O_SYNC flag on dbopen() calls
 - After rename(), fsync() call on directory for faster result
 
* lib/libutil/pw_util.3
 - pw_lock() returns a file descriptor to master password file on success
 
Differential Revision:https://reviews.freebsd.org/D2978
Approved by:  bapt
Sponsored by: Netgate
 
  Modified:
head/lib/libutil/gr_util.c
head/lib/libutil/pw_util.3
head/lib/libutil/pw_util.c
head/usr.sbin/pwd_mkdb/pwd_mkdb.c
 
  Modified: head/lib/libutil/gr_util.c
 
  ==
  --- head/lib/libutil/gr_util.c  Thu Jul  2 16:17:05 2015(r285049)
  +++ head/lib/libutil/gr_util.c  Thu Jul  2 17:30:59 2015(r285050)
  @@ -141,7 +141,7 @@ gr_tmp(int mfd)
  errno = ENAMETOOLONG;
  return (-1);
  }
  -   if ((tfd = mkstemp(tempname)) == -1)
  +   if ((tfd = mkostemp(tempname, O_SYNC)) == -1)
  return (-1);
  if (mfd != -1) {
  while ((nr = read(mfd, buf, sizeof(buf)))  0)
  @@ -318,10 +318,28 @@ gr_copy(int ffd, int tfd, const struct g
   int
   gr_mkdb(void)
   {
  +   int fd;
  +
  if (chmod(tempname, 0644) != 0)
  return (-1);
 
  -   return (rename(tempname, group_file));
  +   if (rename(tempname, group_file) != 0)
  +   return (-1);
  +
  +   /*
  +* Make sure new group file is safe on disk. To improve
  performance we
  +* will call fsync() to the directory where file lies
  +*/
  +   if ((fd = open(group_dir, O_RDONLY|O_DIRECTORY)) == -1)
  +   return (-1);
  +
 
 
 This really is not a real failure!
 Not sure how you would report this but it really is not a failure since the
 rename has completed and you are giving false information back.
 

I disagree.  If you can't open the directory containing the file you
just renamed into that directory, then the assertion that the rename
completed strikes me as speculative at best.  IMO, it's not truly
completed until the fsync() returns success.

-- Ian

 
 
  +   if (fsync(fd) != 0) {
  +   close(fd);
  +   return (-1);
  +   }
  +
  +   close(fd);
  +   return(0);
   }
 
   /*
 
  Modified: head/lib/libutil/pw_util.3
 
  ==
  --- head/lib/libutil/pw_util.3  Thu Jul  2 16:17:05 2015(r285049)
  +++ head/lib/libutil/pw_util.3  Thu Jul  2 17:30:59 2015(r285050)
  @@ -233,7 +233,8 @@ function returns 0 in case of success an
   The
   .Fn pw_lock
   function locks the master password file.
  -It returns 0 in case of success and -1 in case of failure.
  +It returns a file descriptor to master password file in case of success
  +and -1 in case of failure.
   .Pp
   The
   .Fn pw_scan
 
  Modified: head/lib/libutil/pw_util.c
 
  ==
  --- head/lib/libutil/pw_util.c  Thu Jul  2 16:17:05 2015(r285049)
  +++ head/lib/libutil/pw_util.c  Thu Jul  2 17:30:59 2015(r285050)
  @@ -226,7 +226,7 @@ pw_tmp(int mfd)
  errno = ENAMETOOLONG;
  return (-1);
  }
  -   if ((tfd = mkstemp(tempname)) == -1)
  +   if ((tfd = mkostemp(tempname, O_SYNC)) == -1)
  return (-1);
  if (mfd != -1) {
  while ((nr = read(mfd, buf, sizeof(buf)))  0)
 
  Modified: head/usr.sbin/pwd_mkdb/pwd_mkdb.c
 
  ==
  --- head/usr.sbin/pwd_mkdb/pwd_mkdb.c   Thu Jul  2 16:17:05 2015
  (r285049)
  +++ head/usr.sbin/pwd_mkdb/pwd_mkdb.c   Thu Jul  2 17:30:59 2015
  (r285050)
  @@ -51,6 +51,7 @@ __FBSDID($FreeBSD$);
   #include 

Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb

[Mateusz Guzik]

On Thu, Jul 02, 2015 at 12:19:24PM -0600, Ian Lepore wrote:
 On Thu, 2015-07-02 at 20:00 +0200, Ermal Luçi wrote:
  On Thu, Jul 2, 2015 at 7:31 PM, Renato Botelho ga...@freebsd.org wrote:
  
   Author: garga (ports committer)
   Date: Thu Jul  2 17:30:59 2015
   New Revision: 285050
   URL: https://svnweb.freebsd.org/changeset/base/285050
  
   Log:
 When passwd or group information is changed (by pw, vipw, chpass, ...)
 temporary file is created and then a rename() call move it to official
   file.
 This operation didn't have any check to make sure data was written to
   disk
 and if a power cycle happens system could end up with a 0 length passwd
 or group database.
  
 There is a pfSense bug with more infor about it:
  
 https://redmine.pfsense.org/issues/4523
  
 The following changes were made to protect passwd and group operations:
  
 * lib/libutil/gr_util.c:
  - Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
  - After rename(), fsync() call on directory for faster result
  
 * lib/libutil/pw_util.c
  - Replace mkstemp() by mkostemp() with O_SYNC flag to create temp file
  
 * usr.sbin/pwd_mkdb/pwd_mkdb.c
  - Added O_SYNC flag on dbopen() calls
  - After rename(), fsync() call on directory for faster result
  
 * lib/libutil/pw_util.3
  - pw_lock() returns a file descriptor to master password file on 
   success
  
 Differential Revision:https://reviews.freebsd.org/D2978
 Approved by:  bapt
 Sponsored by: Netgate
  
   Modified:
 head/lib/libutil/gr_util.c
 head/lib/libutil/pw_util.3
 head/lib/libutil/pw_util.c
 head/usr.sbin/pwd_mkdb/pwd_mkdb.c
  
   Modified: head/lib/libutil/gr_util.c
  
   ==
   --- head/lib/libutil/gr_util.c  Thu Jul  2 16:17:05 2015(r285049)
   +++ head/lib/libutil/gr_util.c  Thu Jul  2 17:30:59 2015(r285050)
   @@ -141,7 +141,7 @@ gr_tmp(int mfd)
   errno = ENAMETOOLONG;
   return (-1);
   }
   -   if ((tfd = mkstemp(tempname)) == -1)
   +   if ((tfd = mkostemp(tempname, O_SYNC)) == -1)
   return (-1);
   if (mfd != -1) {
   while ((nr = read(mfd, buf, sizeof(buf)))  0)
   @@ -318,10 +318,28 @@ gr_copy(int ffd, int tfd, const struct g
int
gr_mkdb(void)
{
   +   int fd;
   +
   if (chmod(tempname, 0644) != 0)
   return (-1);
  
   -   return (rename(tempname, group_file));
   +   if (rename(tempname, group_file) != 0)
   +   return (-1);
   +
   +   /*
   +* Make sure new group file is safe on disk. To improve
   performance we
   +* will call fsync() to the directory where file lies
   +*/
   +   if ((fd = open(group_dir, O_RDONLY|O_DIRECTORY)) == -1)
   +   return (-1);
   +
  
  
  This really is not a real failure!
  Not sure how you would report this but it really is not a failure since the
  rename has completed and you are giving false information back.
  
 
 I disagree.  If you can't open the directory containing the file you
 just renamed into that directory, then the assertion that the rename
 completed strikes me as speculative at best.  IMO, it's not truly
 completed until the fsync() returns success.
 

Well, it really depends how much you want to nitpick here.

A joke problem is that By the time you open group_dir, it could have ben
renamed/mounted upon/whatever and you are opening a different directory.

A non-joke problem is that the caller cannot distinguish between failed
rename and failed open/sync, making it impossible to perform any kind of
recovery.

I would say the chane should be left as it is. It reportedly fixes the
problem encounterd in the wild and is not that bad

One has to note this entire api and pw(8) are screaming for a rewrite
for a long time now.

 -- Ian
 
  
  
   +   if (fsync(fd) != 0) {
   +   close(fd);
   +   return (-1);
   +   }
   +
   +   close(fd);
   +   return(0);
}
  
/*
  
   Modified: head/lib/libutil/pw_util.3
  
   ==
   --- head/lib/libutil/pw_util.3  Thu Jul  2 16:17:05 2015(r285049)
   +++ head/lib/libutil/pw_util.3  Thu Jul  2 17:30:59 2015(r285050)
   @@ -233,7 +233,8 @@ function returns 0 in case of success an
The
.Fn pw_lock
function locks the master password file.
   -It returns 0 in case of success and -1 in case of failure.
   +It returns a file descriptor to master password file in case of success
   +and -1 in case of failure.
.Pp
The
.Fn pw_scan
  
   Modified: head/lib/libutil/pw_util.c
  
   ==
   --- head/lib/libutil/pw_util.c  Thu Jul  2 16:17:05 2015