ioctl(FS_IOC_SETFLAGS) without ioctl(FS_IOC_GETFLAGS) is wrong

[jirislaby]

The code at:

borg/src/borg/platform/linux.pyx

Lines 142 to 151 in 1c6ef7a

	if open_fd:
	fd = os.open(path, os.O_RDONLY|os.O_NONBLOCK|os.O_NOFOLLOW)
	try:
	if ioctl(fd, FS_IOC_SETFLAGS, &flags) == -1:
	error_number = errno.errno
	if error_number != errno.EOPNOTSUPP:
	raise OSError(error_number, strerror(error_number).decode(), path)
	finally:
	if open_fd:
	os.close(fd)

can remove for example an ext4 flag like EXT4_EXTENTS_FL which results in EOPNOTSUPP. That one is indeed handled above, but this is still incorrect. The flags should be fetched with FS_IOC_GETFLAGS, updated and only then stored back with FS_IOC_SETFLAGS.

This became visible due to a bug in torvalds/linux@474b155. With that commit, ENOTTY is returned instead of EOPNOTSUPP, so borg tests occasionally fail -- depending whether it is possible to convert the inode to no EXT4_EXTENTS_FL or not.

See also: https://bugzilla.suse.com/show_bug.cgi?id=1251048#c3

FTR, strace dump of the failing test:

newfstatat(AT_FDCWD, "/tmp/tmpwsa22bk4/input", {st_mode=S_IFDIR|0777, st_size=4096, ...}, 0) = 0
openat(AT_FDCWD, "/tmp/tmpwsa22bk4/input/file3", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 12
fstat(12, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
ioctl(12, TCGETS2, 0x7ffcdd3c9a20) = -1 ENOTTY (Inappropriate ioctl for device) 
lseek(12, 0, SEEK_CUR)            = 0     
write(12, "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"..., 81920) = 81920 
close(12)                         = 0     
newfstatat(AT_FDCWD, "/tmp/tmpwsa22bk4/input/file3", {st_mode=S_IFREG|0600, st_size=81920, ...}, AT_SYMLINK_NOFOLLOW) = 0
openat(AT_FDCWD, "/tmp/tmpwsa22bk4/input/file3", O_RDONLY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC) = 12
ioctl(12, FS_IOC_SETFLAGS, [FS_NODUMP_FL]) = -1 ENOTTY (Inappropriate ioctl for device) 
close(12)                         = 0     

[ThomasWaldmann]

Interesting find, thanks for reporting it!

Guess the assumption was that we can set the flags to exactly the value that was archived (determined by get_flags).

The general idea of a backup tool is to archive some file data and metadata and to restore exactly the same state later.

So, for flags, that would mean we restore the 0 bits as well as the 1 bits exactly to how they were at archiving time. With that idea, it rather sounds like f = get_flags() at archiving time and set_flags(f) at restore time should do the job (and we would not need to know what all these flags mean and which should be set/reset and which rather should not be modified).

Note: it could be possible that the freebsd and macOS platform code needs a similar fix, needs checking.

[jirislaby]

Guess the assumption was that we can set the flags to exactly the value that was archived (determined by get_flags).

The general idea of a backup tool is to archive some file data and metadata and to restore exactly the same state later.

So for borg proper the functionality is likely fine, as you get_flags at the archiving time and set_flags the stored ones during restore. In that case, only the tests are wrong. But IMO, you don't want to do this for all flags. Some might be modifying internal state of an inode in the FS. Let me ask our FS masters.

At least here:

src/borg/testsuite/__init__.py:32:        platform.set_flags(file.name, stat.UF_NODUMP)
src/borg/testsuite/archiver/__init__.py:229:        platform.set_flags(os.path.join(input_path, "flagfile"), stat.UF_NODUMP)
src/borg/testsuite/archiver/create_cmd_test.py:724:        platform.set_flags(os.path.join(archiver.input_path, "file3"), stat.UF_NODUMP)
src/borg/testsuite/archiver/create_cmd_test.py:1078:    platform.set_flags(os.path.join(archiver.input_path, "nd"), stat.UF_NODUMP)

[jirislaby]

Let me ask our FS masters.

From https://bugzilla.suse.com/show_bug.cgi?id=1251048#c7 (more of ext4-"centric"):

Not all of these flags are fully standardized between filesystems so the behavior might somewhat differ depending on which filesystem you use. Also the flag availability depends on the filesystem features configured during mkfs, kernel features and mount options so let's assume the restore happens to the same filesystem, same kernel, same mount options. Then this should mostly work with couple of exceptions:

1. There are flags which you can read but you cannot set them - EXT4_DIRTY_FL, EXT4_COMPRBLK_FL, EXT4_NOCOMPR_FL, EXT4_ENCRYPT_FL, EXT4_INDEX_FL, EXT4_VERITY_FL, EXT4_INLINE_DATA_FL. Generally these are manipulated in a different way. SETFLAGS ioctl will just ignore values of those flags so whatever you pass is fine but you needn't get the inode in fully identical state as previously.

2. There is EXT4_CASEFOLD_FL which can be modified only on empty directory. Otherwise you get error.

3. There is EXT4_EXTENTS_FL which supports changing file format only in simple cases (more difficult ones need to be handled offline by e2fsck) so setting / clearing the flag may fail.

[jirislaby]

ee8b044 is not correct either, as:

borg/src/borg/platform/linux.pyx

Lines 173 to 175 in 1c6ef7a

	for bsd_flag, linux_flag in BSD_TO_LINUX_FLAGS.items():
	if linux_flags & linux_flag:
	bsd_flags |= bsd_flag

filters the EXT4 flag out :(.

[ThomasWaldmann]

Just as a side note: borg does not know what fs it works with, so the flags processing needs to work independently of the fs type.