[systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

[Lennart Poettering]

On Wed, 08.04.15 21:21, Goffredo Baroncelli (kreijack at libero.it) wrote:

> >> +        const unsigned all_flags = FS_NOATIME_FL|FS_SYNC_FL|FS_DIRSYNC_FL|
> >> +                FS_APPEND_FL|FS_COMPR_FL|FS_NODUMP_FL|FS_EXTENT_FL|
> >> +                FS_IMMUTABLE_FL|FS_JOURNAL_DATA_FL|FS_SECRM_FL|FS_UNRM_FL|
> >> +                FS_NOTAIL_FL|FS_TOPDIR_FL|FS_NOCOW_FL;
> >> +        char *p = item->argument;
> >> +        enum {
> >> +                MODE_ADD,
> >> +                MODE_DEL,
> >> +                MODE_SET
> >> +        } mode = MODE_ADD;
> >> +        unsigned long value = 0, mask = 0;
> > 
> > And now it is "unsigned long", not just "unsigned anymore?
> 
> For 32 bit, the ioctl FS_IOC_GETFLAGS seems to be defined with an
> "int" as argument; for 64bit, the argument is a long; however
> chattr_fd() uses unsigned... Yes this is a mess; probably "int" is a
> good answer

Well, the kernel is a bit confused, but as far as I can see it
generally treats it as "unsigned". Only in the ext234 code there's
this gem that it calls get_user() and pretends it was an int, while
reading it into an unsigned.  btrfs never falls for that, and neither
do most other file systems afaics.

e2fsprogs is also confused. It treats it mostly as "unsigned long",
but then converts it to "int" right before issueing the ioctl. 

I guess the kernel devs haven't understood the concept of "type
safety" yet, or even worse, of "types" ;-)

But anway, I'd stay close to the kernel, so let's make this an
"unsigned".

Flag fields that are signed would be weird anyway...

> >> +        if (lstat(path, &st) == 0 &&
> >> +            !S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) {
> >> +                return 0;
> >> +        }
> > 
> > Why do we need this check? What happens if we apply this onto
> > a device node, or socket, or whatever? 
> 
> I copied this check from the source of chattr; the reason is:
> 
> (from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=152029#10)
> [...]
> >$ lsattr /dev/dri/card0
> >s-S----------- /dev/dri/card0
> >Segmentation fault (core dumped)
> >
> >$ ls -al /dev/dri/card0
> >crw-rw-rw-    1 root     root     226,   0 jun 30 11:12 /dev/dri/card0
> 
> lsattr and chattr can not work against device files, since they use
> ioctl's which are supported by the ext2 filesystem, but which aren't
> supported by normal devices (and unfortunately normal devices don't
> forwawrd the ioctl on to the ext2 filesystem).  In this case, the ioctl
> used by lsattr is apparently also implemented by the DRI device driver,
> but probably does something completely different.  So the returned
> device structure contains garbage, which causes lsattr to core dump
> [...]

I see, this is indeed a real problem (and in fact we might need to fix
this for a couple of other fs related ioctls, too).

Either way, this really should be an fstat() on the opened file and
not a stat() before, due to TOCTTOU.

Anyway, I now made the necessary changes, and changed a few other
things too, and gave it some testing. Please have a look if it still
looks good to you!

Lennart

-- 
Lennart Poettering, Red Hat