From owner-freebsd-fs@FreeBSD.ORG Tue Jan 18 16:06:40 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 CE4DF1065672 for ; Tue, 18 Jan 2011 16:06:40 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx09.syd.optusnet.com.au (fallbackmx09.syd.optusnet.com.au [211.29.132.242]) by mx1.freebsd.org (Postfix) with ESMTP id 554528FC18 for ; Tue, 18 Jan 2011 16:06:39 +0000 (UTC) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by fallbackmx09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p0IEIB23004885 for ; Wed, 19 Jan 2011 01:18:11 +1100 Received: from c122-106-165-206.carlnfd1.nsw.optusnet.com.au (c122-106-165-206.carlnfd1.nsw.optusnet.com.au [122.106.165.206]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p0IEI8jq027597 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 19 Jan 2011 01:18:09 +1100 Date: Wed, 19 Jan 2011 01:18:08 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Kurtsou In-Reply-To: <20110118041447.GA2087@tops.skynet.lt> Message-ID: <20110118234350.R1077@besplex.bde.org> References: <20110118041447.GA2087@tops.skynet.lt> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: Garrett Cooper , 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 16:06:40 -0000 On Tue, 18 Jan 2011, Gleb Kurtsou wrote: > On (17/01/2011 16:32), Garrett Cooper wrote: >> Hi FS folks, >> >> Originally I did this to determine why lchflags had an int >> argument and chflags/fchflags had unsigned long arguments, and I ran chflags/fchflags() have unsigned long args since these were wrong in 4.4BSD-Lite, and they still have unsigned long args since it is easier not to disturb sleeping bugs. lchflags() has an int arg since it was implemented in FreeBSD much later than the others, and the type error in the others was apparently intentionally avoided. (I thought the non-error for lchflags() was copied from NetBSD, but NetBSD seems to have always had u_long for all three). As you found, the kernel wants everything to be an int, and setfflags() still only accepts an int. Thus it is an error to pretend that the syscalls take args of other types. Larger types would be handled correctly if syscalls.master actually pseudo-declared ths syscalls correctly, but it still says int as in 4.4BSD-Lite for chflags/fchflags(). FreeBSD generates args structs containing the actual types from the data in syscalls.master. If the latter is correct, then the correct conversions will be done without further type puns, by normal C conversions, e.g., setfflags(..., uap->flag), where `flags' has whatever type it has and the function call converts to int. Plain ints work correctly except possibly for invalid args since the sign bit is not used by any supported flags value. >> into an interesting mini-project (by accident) after I found a few >> bugs lurking in the vfs_syscalls.c layer of the kernel. >> >> 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. The downcast in the call to setfflags() is safe, but the type puns given by the wrong types in syscalls.master are not. I think the big-endian 64-bit long case is the only case seriously broken by these puns, but FreeBSD doesn't notice since it doesn't support any big-endian 64-bit arches. >> 3. There's a sizing/signedness discrepancy in a few other structures >> with fixed-width data types (uint32_t). There are zillions of these, and with missing consts. Not a single vfs syscall supports its path arg being declared as const in syscalls.master. Adding consts there would cause more problems than fixing type mismatches like the ones for fflags, since lower layers are almost perfectly devoid of "const", and "downcasting" for "const" involves removing it, and gcc would complain about that although it is even safer than downcasting to int for vfs syscalls. >> Solution: >> ... >> >> 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). No, NetBSD has data in syscalls.master that actually matches the arg types for the user API for *chflags() (it even has "const" for open()), so, it does all the downcasting correctly and doesn't have any serious 64-bit problems. At least in 2005, its set_fflags() is spelled change_flags() and takes a u_long flags, and this seems to reach VOP_SETATTR() correctly via "u_long va_flags;" (even 4.4BSD-Lite and FreeBSD have "u_long va_flags"). Even if the top flags bits were clipped early as in FreeBSD, there would only be minor problems from not detecting garbage in these bits (including the ability of applications to invoke undefined behaviour in the kernel, by passing garbage top bits that cause integer overflow on blind conversion to int). Everone uses u_int32_t for st_flags, except u_int32_t is spelled fflags_t in FreeBSD, but this doesn't cause annoy problems since it is large enough and read-only. >> 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). Was consistently u_long in 2005, except where it must be spelled "unsigned long" for namespace reasons. > You've changed syscalls definitions but didn't add backward > compatibility shims for kernel, libc and freebsd32. Probably not needed, especially of garbage in top bits is not checked for (and checking that is not really needed. There are SF_SETTABLE and UF_SETTABLE masks which would cause garbage top bits to be silently ignored if they are masked before checking). Bit the patch doesn't seem to touch syscalls.master. That risks increasing the problem unless everything is changed to int to match there. Even if you change like that, old 64-bit binaries still passing u_long will continue to work, since there are no big-endian ones and ints are padded to 64 bits in the ABI (the big-endian case breaks by padding at the wrong end). > 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 Enlarging sizes is harder :-). >> Index: include/protocols/dumprestore.h >> =================================================================== >> --- include/protocols/dumprestore.h (revision 217362) >> +++ include/protocols/dumprestore.h (working copy) >> @@ -95,7 +95,7 @@ >> int64_t c_mtime; /* last modified time, seconds */ >> int32_t c_extsize; /* external attribute size */ >> int32_t c_spare4[6]; /* old block pointers */ >> - u_int32_t c_file_flags; /* status flags (chflags) */ >> + fflags_t c_file_flags; /* status flags (chflags) */ > The struct represents on-disk protocol description, and thus shouldn't be changed. Indeed. fflags_t is essentially only the type used in struct stat's API and ABI. It started as u_int32_t too. Changing the API and ABI for all *chflags() to use it is probably safe too. >> Index: sbin/restore/tape.c >> =================================================================== >> --- sbin/restore/tape.c (revision 217362) >> +++ sbin/restore/tape.c (working copy) >> @@ -558,7 +558,7 @@ >> int >> extractfile(char *name) >> { >> - int flags; >> + fflags_t flags; >> uid_t uid; >> gid_t gid; >> mode_t mode; Unclear from the patch alone if it needs to match dump's ABI. Probably not. >> Index: sys/kern/vfs_syscalls.c >> =================================================================== >> --- sys/kern/vfs_syscalls.c (revision 217362) >> +++ sys/kern/vfs_syscalls.c (working copy) >> @@ -96,7 +96,7 @@ >> static int getutimes(const struct timeval *, enum uio_seg, struct timespec *); >> static int setfown(struct thread *td, struct vnode *, uid_t, gid_t); >> static int setfmode(struct thread *td, struct vnode *, int); >> -static int setfflags(struct thread *td, struct vnode *, int); >> +static int setfflags(struct thread *td, struct vnode *, fflags_t); >> static int setutimes(struct thread *td, struct vnode *, >> const struct timespec *, int, int); >> static int vn_access(struct vnode *vp, int user_flags, struct ucred *cred, Better to change this to whatever va_flags is. The latter can remain as u_long. >> @@ -2695,8 +2695,8 @@ >> */ >> #ifndef _SYS_SYSPROTO_H_ >> struct chflags_args { >> - char *path; >> - int flags; >> + char *path; >> + fflags_t flags; >> }; >> #endif >> int Now has excessive indentation. This was consistently broken with syscalls.master. Similarly for others. >> Index: sys/ufs/ufs/dinode.h >> =================================================================== >> --- sys/ufs/ufs/dinode.h (revision 217362) >> +++ sys/ufs/ufs/dinode.h (working copy) >> @@ -140,7 +140,7 @@ >> int32_t di_birthnsec; /* 76: Inode creation time. */ >> int32_t di_gen; /* 80: Generation number. */ >> u_int32_t di_kernflags; /* 84: Kernel flags. */ >> - u_int32_t di_flags; /* 88: Status flags (chflags). */ >> + fflags_t di_flags; /* 88: Status flags (chflags). */ > Please revert for the same reason. Indeed. Bruce