Date: Wed, 19 Jan 2011 03:49:03 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <yanegomi@gmail.com> Cc: fs@FreeBSD.org Subject: Re: Inconsistency and bug with *chflags functions Message-ID: <20110119030959.I2099@besplex.bde.org> In-Reply-To: <AANLkTikfe%2BxTzRyHDwLRhg6_sg4oM0jM%2Bf_jnY-CPz7g@mail.gmail.com> References: <AANLkTintj_eM=aRF4pO8PcT8VXvVJ=Booxag%2BwBg3gaX@mail.gmail.com> <20110118041447.GA2087@tops.skynet.lt> <20110118234350.R1077@besplex.bde.org> <AANLkTikfe%2BxTzRyHDwLRhg6_sg4oM0jM%2Bf_jnY-CPz7g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--0-781286865-1295369343=:2099
Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
On Tue, 18 Jan 2011, Garrett Cooper wrote:
> On Tue, Jan 18, 2011 at 6:18 AM, Bruce Evans <brde@optusnet.com.au> wrote=
:
>> On Tue, 18 Jan 2011, Gleb Kurtsou wrote:
>
> ...
>
>> As you found, the kernel wants everything to be an int, and setfflags()
>> still only accepts an int. ...
[Quote trimmed at the first hard \xa0]
> Signedness isn't important for this flag. Only width and data
> alignment as you suggested elsewhere.
Not important, but it allows undefined behaviour.
>>>> 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. =A0I 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.
>
> This is incorrect unfortunately. The newest port of the PowerPC family
> (Sony's PS3) is actually Big Endian, and many of the other archs like
> ARM and MIPS of course are mixed endian :(... so I think that bugs
> like these will become more apparent as time goes on.
I think these are still unsupported, since chflags() cannot work on them :-=
).
>>> The struct represents on-disk protocol description, and thus shouldn't =
be
>>> changed.
>>
>> Indeed. =A0fflags_t is essentially only the type used in struct stat's A=
PI and
>> ABI. =A0It started as u_int32_t too. =A0Changing the API and ABI for all
>> *chflags() to use it is probably safe too.
>
> Which was my reasoning, but I was a bit uneasy about this.
You can't really start from 1 read-only API. This API only tells us that
there are at most 32 flags.
>
>>>> Index: sbin/restore/tape.c
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>> --- sbin/restore/tape.c (revision 217362)
>>>> +++ sbin/restore/tape.c (working copy)
>>>> @@ -558,7 +558,7 @@
>>>> =A0int
>>>> =A0extractfile(char *name)
>>>> =A0{
>>>> - =A0 =A0 =A0 int flags;
>>>> + =A0 =A0 =A0 fflags_t flags;
>>>> =A0 =A0 =A0 =A0uid_t uid;
>>>> =A0 =A0 =A0 =A0gid_t gid;
>>>> =A0 =A0 =A0 =A0mode_t mode;
>>
>> Unclear from the patch alone if it needs to match dump's ABI. =A0Probabl=
y not.
>
> It does (there were some bits missing that I need to submit in a new patc=
h).
I checked it and don't see any problems except possibly compiler
warnings about type matches that turn out to be harmless. The value
just gets converted to int when read from the dump (was u_int32_t
there) and from int when passed to chflags() (was u_long there).
"int" is large enough unless the dump has garbage flags.
>>>> Index: sys/kern/vfs_syscalls.c
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>> --- sys/kern/vfs_syscalls.c =A0 =A0 (revision 217362)
>>>> +++ sys/kern/vfs_syscalls.c =A0 =A0 (working copy)
>>>> @@ -96,7 +96,7 @@
>>>> =A0static int getutimes(const struct timeval *, enum uio_seg, struct
>>>> timespec *);
>>>> =A0static int setfown(struct thread *td, struct vnode *, uid_t, gid_t)=
;
>>>> =A0static 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);
>>>> =A0static int setutimes(struct thread *td, struct vnode *,
>>>> =A0 =A0 const struct timespec *, int, int);
>>>> =A0static int vn_access(struct vnode *vp, int user_flags, struct ucred
>>>> *cred,
>>
>> Better to change this to whatever va_flags is. =A0The latter can remain =
as
>> u_long.
>
> But silent value truncation is bad, right :(?
Yes, but we always did it. You don't want to make ABI problems larger by
adding strictness now.
I checked what ufs does. It is sloppy for root -- allows anything in
the user-supplied to be put into on-disk flags (after truncation to 32
bits). For non-root, it only allows user flags to be changed, but the
mask for this includes 11 flags that have no assigned meaning. Users
can use this to store 11 bits of data in an empty file :-).
%=20
% Script started on Wed Jan 19 03:36:19 2011
% ttyv6:bde@besplex:/tmp/q> >z
% ttyv6:bde@besplex:/tmp/q> chflags 177777 z
% ttyv6:bde@besplex:/tmp/q> ls -lo z
% -rw-r--r-- 1 bde wheel uappnd,uchg,nodump,opaque,uunlnk 0 Jan 19 03:36=
z
% ttyv6:bde@besplex:/tmp/q> /usr/bin/stat z
% 256768 1186 -rw-r--r-- 1 bde wheel 0 0 "Jan 19 03:36:21 2011" "Jan 19 03:=
36:21 2011" "Jan 19 03:36:29 2011" "Jan 1 10:00:00 1970" 8192 0 0xffff z
0xffff is my 11 bites of data (5 flags and 11 bits not reported by ls
or flagstostr() generally.
% ttyv6:bde@besplex:/tmp/q> chflags 377777 z
% chflags: z: Operation not permitted
Setting 1 system bit is correctly not allowed.
% ttyv6:bde@besplex:/tmp/q> chflags 0 z
% ttyv6:bde@besplex:/tmp/q> rm z
% ttyv6:bde@besplex:/tmp/q> exit
%=20
% Script done on Wed Jan 19 03:37:00 2011
> ...
>
>> Now has excessive indentation. =A0This was consistently broken with
>> syscalls.master. =A0Similarly for others.
>
> Eh...? Wouldn't it be worse if fflags_t
The quote isn't there and the reply is truncated :-).
Indentation is 8 columns after a type. I don't like indenting all
the variable names because 1 type name is too verbose.
>>>> Index: sys/ufs/ufs/dinode.h
>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>>> --- sys/ufs/ufs/dinode.h =A0 =A0 =A0 =A0(revision 217362)
>>>> +++ sys/ufs/ufs/dinode.h =A0 =A0 =A0 =A0(working copy)
>>>> @@ -140,7 +140,7 @@
>>>> =A0 =A0 =A0 =A0int32_t =A0 =A0 =A0 =A0 di_birthnsec; =A0 /* =A076: Ino=
de creation time. */
>>>> =A0 =A0 =A0 =A0int32_t =A0 =A0 =A0 =A0 di_gen; =A0 =A0 =A0 =A0 /* =A08=
0: Generation number. */
>>>> =A0 =A0 =A0 =A0u_int32_t =A0 =A0 =A0 di_kernflags; =A0 /* =A084: Kerne=
l flags. */
>>>> - =A0 =A0 =A0 u_int32_t =A0 =A0 =A0 di_flags; =A0 =A0 =A0 /* =A088: St=
atus flags (chflags).
>>>> */
>>>> + =A0 =A0 =A0 fflags_t =A0 =A0 =A0 =A0di_flags; =A0 =A0 =A0 /* =A088: =
Status flags (chflags).
>>>> */
Unreadble due to MUA mangling of spaces.
>>> Please revert for the same reason.
>>
>> Indeed.
>
> I'm still confused as to why silent type [width] parity issues with
> UFS is a good thing, especially when this change serves to unite
> everything under the same bit-width type (well, minus the syscalls,
> but that was done because it's easier to deal with function call
> breakage as opposed to data interpretation breakage when reading off
> the disk.
ffs can only hold 32-bit flags no matter what fflags_t is. If you
want to change this in ffs, then you have to write ffs3, and there are
more important things to change in it then. OTOH, changing fflags_t
wont affect ffs, even if some new bits in it are actually used. ffs
just won't understand these bits and will return an error for attempts
to set them, at least when it is fixed to not silently ignore them.
(Actually using old bits is more of a problem. Users may already be
using them for storing data :-)).
Bruce
--0-781286865-1295369343=:2099--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110119030959.I2099>