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

[Goffredo Baroncelli]

On 2015-04-08 20:36, Lennart Poettering wrote:
> On Mon, 16.03.15 20:33, Goffredo Baroncelli (kreijack at libero.it) wrote:
> 
> Sorry for warming up this old thread, but a few issues I see with this?
> 
>>          RECURSIVE_RELABEL_PATH = 'Z',
>> +        SET_ATTRIB = 'h',
>> +        RECURSIVE_SET_ATTRIB = 'H',
>>  } ItemType;
>>  
>>  typedef struct Item {
>> @@ -108,12 +111,15 @@ typedef struct Item {
>>          usec_t age;
>>  
>>          dev_t major_minor;
>> +        int attrib_value;
>> +        int attrib_mask;
> 
> I thought this was unsigned now?

Sew below



> 
>>  
>> +static int get_attrib_from_arg(Item *item) {
>> +        static const unsigned attributes[] = {
>> +                [(uint8_t)'A'] = FS_NOATIME_FL,    /* do not update atime */
>> +                [(uint8_t)'S'] = FS_SYNC_FL,      /* Synchronous updates */
>> +                [(uint8_t)'D'] = FS_DIRSYNC_FL,   /* dirsync behaviour (directories only) */
>> +                [(uint8_t)'a'] = FS_APPEND_FL,    /* writes to file may only append */
>> +                [(uint8_t)'c'] = FS_COMPR_FL,     /* Compress file */
>> +                [(uint8_t)'d'] = FS_NODUMP_FL,    /* do not dump file */
>> +                [(uint8_t)'e'] = FS_EXTENT_FL,    /* Top of directory hierarchies*/
>> +                [(uint8_t)'i'] = FS_IMMUTABLE_FL, /* Immutable file */
>> +                [(uint8_t)'j'] = FS_JOURNAL_DATA_FL, /* Reserved for ext3 */
>> +                [(uint8_t)'s'] = FS_SECRM_FL,     /* Secure deletion */
>> +                [(uint8_t)'u'] = FS_UNRM_FL,      /* Undelete */
>> +                [(uint8_t)'t'] = FS_NOTAIL_FL,    /* file tail should not be merged */
>> +                [(uint8_t)'T'] = FS_TOPDIR_FL,    /* Top of directory hierarchies*/
>> +                [(uint8_t)'C'] = FS_NOCOW_FL,     /* Do not cow file */
>> +        };
> 
> Hmm, that's quite a sparse array! It wastes 116 entries, just to
> store 13 entries... I think this should really be an array fo structs
> with the chars and their flag, which we iterate through... 

I accepted a suggestion of Zbyszek; this solution avoids to loop over the list... 
> 
>> +        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

> 
>> +
>> +static int path_set_attrib(Item *item, const char *path) {
>> +        _cleanup_close_ int fd = -1;
>> +        int r;
>> +        unsigned f;
>> +        struct stat st;
>> +
>> +        /* do nothing */
>> +        if (item->attrib_mask == 0 || !item->attrib_set)
>> +                return 0;
>> +        /*
>> +         * It is OK to ignore an lstat() error, because the error
>> +         * will be catch by the open() below 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 really don#t like this seperate lstat() + open(). If it all it
> should be open() + fstat(), to avoid TTOCTOU issues...
> 
>> +        fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
>> +
>> +        if (fd < 0)
>> +                return log_error_errno(errno, "Cannot open \"%s\": %m", path);
>>  
>> +        case SET_ATTRIB:
>> +        case RECURSIVE_SET_ATTRIB:
>> +                if (!i.argument) {
>> +                        log_error("[%s:%u] Set attrib requires
>> argument.", fname, line);
> 
> Please don't abbreviate words like "attrib" in human language...
> 
> Lennart
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5