From owner-freebsd-fs@FreeBSD.ORG Tue Jan 18 04:47:48 2011 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0C014106566B for ; Tue, 18 Jan 2011 04:47:48 +0000 (UTC) (envelope-from yanegomi@gmail.com) Received: from mail-ww0-f50.google.com (mail-ww0-f50.google.com [74.125.82.50]) by mx1.freebsd.org (Postfix) with ESMTP id 935628FC1A for ; Tue, 18 Jan 2011 04:47:47 +0000 (UTC) Received: by wwf26 with SMTP id 26so5966722wwf.31 for ; Mon, 17 Jan 2011 20:47:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=gLKShVgqvWGIIa5LWJuGSDI481Adf9DtghTPy0skJPs=; b=nXpdqkQISh3i/uBoKclIPL9mK0ZjAfR9TbRRL4GR5r38PFROgU0NMwTmNRthIAogku K4rXt5I46QTVhfqo0dg+U/r0E93j7nuTrHUWhu6yAOAUZ+k+hfruZyhil9zBFZkDM4Cs VMEZJGq5fdtRqKCAWXw99+JWG0kB7KZxrMpRY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=WYVCTcb5HdskAmmY4DcEhDkutCln8B0US0KAGJLC08OKg42ohCP+0y01Dr8NTNGXWe 1Wqr9pWRA31x8KOHHihSlAWT/HKNi8Ibgu9ZjdYtXlK41yYJPiQmPWve6CgKPpTxrmg2 M7n3H5Y6BaWW6hJ/d0GNwtitT9gU7aVuK6sTI= MIME-Version: 1.0 Received: by 10.216.141.75 with SMTP id f53mr4202386wej.16.1295326066420; Mon, 17 Jan 2011 20:47:46 -0800 (PST) Received: by 10.216.254.226 with HTTP; Mon, 17 Jan 2011 20:47:46 -0800 (PST) In-Reply-To: <20110118041447.GA2087@tops.skynet.lt> References: <20110118041447.GA2087@tops.skynet.lt> Date: Mon, 17 Jan 2011 20:47:46 -0800 Message-ID: From: Garrett Cooper To: Gleb Kurtsou Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: fs@freebsd.org Subject: Re: Inconsistency and bug with *chflags functions X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jan 2011 04:47:48 -0000 Hi Gleb! On Mon, Jan 17, 2011 at 8:14 PM, Gleb Kurtsou wrot= e: > On (17/01/2011 16:32), Garrett Cooper wrote: >> Hi FS folks, >> >> =A0 =A0 Originally I did this to determine why lchflags had an int >> argument and chflags/fchflags had unsigned long arguments, and I ran >> into an interesting mini-project (by accident) after I found a few >> bugs lurking in the vfs_syscalls.c layer of the kernel. >> >> =A0 =A0 So here's the deal... >> >> Problems: >> 1. chflags/fchflags claim that the flags argument is an unsigned long, >> whereas lchflags claims it's an int. This is inconsistent. >> 2. The kernel interfaces are all implicitly downcasting the flags >> argument to int. >> 3. There's a sizing/signedness discrepancy in a few other structures >> with fixed-width data types (uint32_t). >> >> Solution: >> 1. I opted to convert fflags_t to uint32_t and convert all references >> which did direct conversions to and from st_flags to fflags_t to avoid >> 32-bit / 64-bit biarch issues. I downgraded it to uint32_t as we're no >> where near the limit for the number of usable flags to *chflags(2). >> 2. *chflags now uses fflags_t instead of int/unsigned long. >> 3. c_file_flags in dumprestore.h was changed to be in sync with st_flags= . >> 4. di_flags in ufs/ufs/dinode.h was changed to be in sync with st_flags. >> >> Compatibility: >> 1. NetBSD uses unsigned long in their chflags calls (both in kernel >> and userland) so they're more consistent than we are by not having >> mixed flags calling convention like us, but uses uint32_t in their >> data structures (like we do), so they have a 32-bit/64-bit biarch bug >> (again like we do). >> 2. OpenBSD is using unsigned int, so I assume that their kernel layer >> is also using unsigned int (I am basing this purely off the manpage as >> I haven't looked at their sources, but I could be wrong). > > Hi Garrett, > > You've changed syscalls definitions but didn't add backward > compatibility shims for kernel, libc and freebsd32. Please enlighten me on how to do that (docs are fine) and I'll go ahead and add the compatibility shims. I'm not a committer so I don't yet know these things :). The only thing is that the old behavior on biarch platforms like amd64 with COMPAT_FREEBSD32, etc was broken so I don't know if there's any value in emulating broken behavior. Straight 32-bit and straight 64-bit archs didn't have this problem. > I'm working on changing ino_t to 64 bits and nlink_t to 32 bits, we > could join the effort, as projects look related. My old patch is > available here: > https://github.com/downloads/glk/freebsd-ino64/freebsd-ino64-patch.tgz Hmmm... I'll take a look at that. FWIW, I was avoiding bumping fflags_t to uint64_t because it would change alignment of kernel structures, but I'll take your advice in kind for resolving this problem. There are other ways to avoid this by downcasting (or just casting between types in general), which is fine today but I thought this solution was simple because it was easier to root out affected areas. Either way, I know that this method is a bit of a damned if you do, damned if you don't because it restricts us from future additions if fflags_t goes exceeds 2^32, but that's still the case today if the field changes in the on-disk structures anyhow. > Please find comments to the patch below. Ok :)! >> =A0 =A0 I'm running make universe just to see if it will barf in the >> build, but would someone please review this change (I made the change >> as minimal as possible for ease of review) and provide feedback? ... >> - =A0 =A0 =A0 =A0 =A0 =A0 u_int32_t c_file_flags; =A0 =A0 /* status flag= s (chflags) */ >> + =A0 =A0 =A0 =A0 =A0 =A0 fflags_t c_file_flags; =A0 =A0 =A0/* status fl= ags (chflags) */ > The struct represents on-disk protocol description, and thus shouldn't be= changed. fflags_t in this case is uint32_t, so this is a drop-in-place replacement -- or are you arguing the type more than the width of the type? >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 int32_t c_spare5[2]; =A0 =A0 =A0 =A0/* old b= locks, generation number */ ... >> - =A0 =A0 u_int32_t =A0 =A0 =A0 di_flags; =A0 =A0 =A0 /* =A088: Status f= lags (chflags). */ >> + =A0 =A0 fflags_t =A0 =A0 =A0 =A0di_flags; =A0 =A0 =A0 /* =A088: Status= flags (chflags). */ > Please revert for the same reason. My response here is the same as above. Thanks! -Garrett