Date: Tue, 18 Jan 2011 12:54:15 +0200 From: Gleb Kurtsou <gleb.kurtsou@gmail.com> To: Garrett Cooper <yanegomi@gmail.com> Cc: fs@freebsd.org Subject: Re: Inconsistency and bug with *chflags functions Message-ID: <20110118105415.GA73120@tops.skynet.lt> In-Reply-To: <AANLkTinEMQQccZrY6d1VTFOHVU9nMG0DdOLEk7Q-S8QS@mail.gmail.com> References: <AANLkTintj_eM=aRF4pO8PcT8VXvVJ=Booxag%2BwBg3gaX@mail.gmail.com> <20110118041447.GA2087@tops.skynet.lt> <AANLkTinEMQQccZrY6d1VTFOHVU9nMG0DdOLEk7Q-S8QS@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On (17/01/2011 20:47), Garrett Cooper wrote: > Hi Gleb! > > On Mon, Jan 17, 2011 at 8:14 PM, Gleb Kurtsou <gleb.kurtsou@gmail.com> 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 > >> 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. > >> 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 :). It's unlikely to be documented. You can't just change syscall definition, you should add a new one and mark old COMPAT8 (in this particular case). Implement kernel level wrappers, etc. Symbol versioning within libc is rather different story, I'd suggest you take a look at my patch it should be straightforward. > 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. Yes, they are broken, but due to amd64 being little ending and flag argument being bit mask, I think both chflags and fchaflags actually work, although it should also be fixed, imho. Wrapper won't be needed for the new version (with fflags_t). > > 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. 32 bits should be good enough. Extended attributes are getting more widely used nowadays. > > Please find comments to the patch below. > > Ok :)! > > >> 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? > > ... > > >> - 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. > > 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? It's common practice, this way fflags_t could be changed to different type in the future, but binary compatibility (dump/restore protocol here) will be preserved. As it happened with uid_t, pid_t, off_t, or taking place with ino_t and nlink_t. No typedefs are used in struct ufs2_dinode or protocols/dumprestore.h (ino_t is being fixed). > >> int32_t c_spare5[2]; /* old blocks, generation number */ > > ... > > >> - u_int32_t di_flags; /* 88: Status flags (chflags). */ > >> + fflags_t di_flags; /* 88: Status flags (chflags). */ > > Please revert for the same reason. > > My response here is the same as above. > > Thanks! > -Garrett
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110118105415.GA73120>