From owner-svn-src-head@freebsd.org Thu Jul 2 18:37:48 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 41DA7993D0A; Thu, 2 Jul 2015 18:37:48 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wi0-x229.google.com (mail-wi0-x229.google.com [IPv6:2a00:1450:400c:c05::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id CCB661F7B; Thu, 2 Jul 2015 18:37:47 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by wiar9 with SMTP id r9so109780149wia.1; Thu, 02 Jul 2015 11:37:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=Wpa62+2fwvLVmty+7h6x+IwFH6lIOrhv6LwXCj7GhaY=; b=P2UVANcb/lVRmwidqq3ua3jahVwZJj5tccLKwjKkU8sSzz+QYtx65COXWbeu1aQK6+ so+lrki7WbP+/2OzB1ye9rCNKFfxMGAcdvVoE8kUpDAs4rB51Lq84eqf/+8ErZRP7E4d qKM7zQshJ3z9aPj33JPvebuH9R8BYU5+OuBmWSCLsCos1i/e99hKodCo9cCon15Npxv1 YYGS3Z8bfBBNS/jJ4VRiCiws2z4c1MIgqmbihgvggyU4ZiVYAygY0/6mdzBRcoVpMbSq PGHpIWX0G2r7OzhrbSpWXc6W5+tuSo6l36tjFUeriH5WU2nOSH7SlHAktH5QZctCuMwV v0qA== X-Received: by 10.180.84.202 with SMTP id b10mr19249824wiz.23.1435862266290; Thu, 02 Jul 2015 11:37:46 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id lv8sm2737386wjb.41.2015.07.02.11.37.43 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 02 Jul 2015 11:37:44 -0700 (PDT) Date: Thu, 2 Jul 2015 20:37:41 +0200 From: Mateusz Guzik To: Ian Lepore Cc: Ermal =?utf-8?B?THXDp2k=?= , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Renato Botelho Subject: Re: svn commit: r285050 - in head: lib/libutil usr.sbin/pwd_mkdb Message-ID: <20150702183741.GA7666@dft-labs.eu> References: <201507021731.t62HV074085188@repo.freebsd.org> <1435861164.1648.129.camel@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1435861164.1648.129.camel@freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Jul 2015 18:37:48 -0000 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 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 (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 > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -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); > > > if (dp == NULL) > > > error(buf); > > > > > > clean = FILE_SECURE; > > > cp(sbuf2, sbuf, PERM_SECURE); > > > sdp = dbopen(sbuf, > > > - O_RDWR|O_EXCL, PERM_SECURE, DB_HASH, &openinfo); > > > + O_RDWR|O_EXCL|O_SYNC, PERM_SECURE, DB_HASH, &openinfo); > > > if (sdp == NULL) > > > error(sbuf); > > > > > > @@ -291,13 +292,13 @@ main(int argc, char *argv[]) > > > method = 0; > > > } else { > > > dp = dbopen(buf, > > > - O_RDWR|O_CREAT|O_EXCL, PERM_INSECURE, DB_HASH, > > > &openinfo); > > > + O_RDWR|O_CREAT|O_EXCL|O_SYNC, PERM_INSECURE, DB_HASH, > > > &openinfo); > > > if (dp == NULL) > > > error(buf); > > > clean = FILE_INSECURE; > > > > > > sdp = dbopen(sbuf, > > > - O_RDWR|O_CREAT|O_EXCL, PERM_SECURE, DB_HASH, > > > &openinfo); > > > + O_RDWR|O_CREAT|O_EXCL|O_SYNC, PERM_SECURE, DB_HASH, > > > &openinfo); > > > if (sdp == NULL) > > > error(sbuf); > > > clean = FILE_SECURE; > > > @@ -721,13 +722,27 @@ void > > > mv(char *from, char *to) > > > { > > > char buf[MAXPATHLEN]; > > > + char *to_dir; > > > + int to_dir_fd = -1; > > > > > > - if (rename(from, to)) { > > > + /* > > > + * Make sure file is safe on disk. To improve performance we will > > > call > > > + * fsync() to the directory where file lies > > > + */ > > > + if (rename(from, to) != 0 || > > > + (to_dir = dirname(to)) == NULL || > > > + (to_dir_fd = open(to_dir, O_RDONLY|O_DIRECTORY)) == -1 || > > > + fsync(to_dir_fd) != 0) { > > > int sverrno = errno; > > > (void)snprintf(buf, sizeof(buf), "%s to %s", from, to); > > > errno = sverrno; > > > + if (to_dir_fd != -1) > > > + close(to_dir_fd); > > > error(buf); > > > } > > > + > > > + if (to_dir_fd != -1) > > > + close(to_dir_fd); > > > } > > > > > > void > > > > > > -- > > > Ermal > > > > > > _______________________________________________ > svn-src-all@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/svn-src-all > To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" -- Mateusz Guzik