Bug 50861 - Btrfs sometimes ignore umask and create world writable files
Summary: Btrfs sometimes ignore umask and create world writable files
Status: RESOLVED CODE_FIX
Alias: None
Product: File System
Classification: Unclassified
Component: btrfs (show other bugs)
Hardware: All Linux
: P1 high
Assignee: Filipe Brandenburger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-21 17:23 UTC by rcs
Modified: 2013-02-23 22:14 UTC (History)
3 users (show)

See Also:
Kernel Version: 3.2.1, 3.4.2, 3.5.4, 3.6.2, 3.6.6, 3.7
Subsystem:
Regression: No
Bisected commit-id:


Attachments
Test program in C: creates 1000000 empty files (206 bytes, text/plain)
2012-11-21 17:23 UTC, rcs
Details
Patch that fixes this issue (460 bytes, patch)
2012-11-29 08:11 UTC, Filipe Brandenburger
Details | Diff
Script to check whether the bug is present (728 bytes, application/x-sh)
2012-11-30 04:44 UTC, Filipe Brandenburger
Details

Description rcs 2012-11-21 17:23:41 UTC
Created attachment 86901 [details]
Test program in C: creates 1000000 empty files

On my systems, which use btrfs subvolumes, i found bunch of world writable files.
Systems have umask 0022.

After investigation i found that this problem is related to btrfs.

Steps to Reproduce:

1) create 1000000 files
1a) compile attached program
gcc -O2 mkfiles.c
1b) run
umask 0022
./a.out

2) count world writable files
find . -type f -perm -o+w | wc -l

Actual Results:
last command return non zero value

Expected Results:
last command should return 0

Tested on Gentoo kernels:
32bit uniprocessor
3.6.2-gentoo

64bit SMP
3.4.2-hardened
3.5.4-hardened-r1
3.6.6-gentoo
Comment 1 Filipe Brandenburger 2012-11-29 03:43:13 UTC
Hi,

I managed to reproduce this on 3.7-rc7+ from git.

I added some traces and noticed a difference when using ">" redirection (equivalent to what you're doing) and using "touch" (which changes the mtime of the file).


When using "echo -n >test":

> Nov 28 08:56:18 kdevel kernel: [   70.862440] btrfs_create: i_ino=259
> i_mode=100664 mode=100666 [test]
> Nov 28 08:56:18 kdevel kernel: [   70.862830]
> btrfs_delayed_update_inode(begin): i_ino=259 i_mode=100664
> Nov 28 08:56:34 kdevel kernel: [   86.250849]
> btrfs_delayed_update_inode(delayed_node->inode_dirty): i_ino=258 i_mode=40775
> Nov 28 08:56:18 kdevel kernel: [   70.863320] fill_stack_inode_item:
> i_ino=259 i_mode=100664


When using "touch test" (on another directory where it doesn't exist):

> Nov 28 08:56:34 kdevel kernel: [   86.249028] fill_inode_item: i_ino=260
> i_mode=100666
> Nov 28 08:56:34 kdevel kernel: [   86.253491] btrfs_create: i_ino=260
> i_mode=100664 mode=100666 [0]


It seems that when the file is created, the mode is correctly set on the in-memory inode but it's not marked as dirty so after a filesystem unmount/re-mount or when the in-memory inode is flushed the permissions will appear wrong again...

Some actions such as "touch" (modifying the mtime) will make the in-memory inode dirty so forcing a flush...

I'll look further to see why this is happening. BTW, do you know of a kernel where this problem doesn't occur? That would help with bisect to find which commit might have broken it...

Cheers,
Filipe
Comment 2 Filipe Brandenburger 2012-11-29 08:11:47 UTC
Created attachment 87721 [details]
Patch that fixes this issue

This patch fixes this issue by calling btrfs_update_inode() in btrfs_create(). This ensures that the change in i_mode done in btrfs_init_acl() to mask out the umask will get into the btrfs_inode on disk.

I used the test program. Using the latest kernel from git without this patch I could easily reproduce this issue (easy by unmounting and remounting the filesystem shortly after creating a few files) but with the patch I could no longer reproduce it. I used the test program in this bug to create 1,000,000 files and didn't reproduce it either.

I think this might potentially be the correct approach since most of the other calls (like mkdir, symlink, etc.) are calling btrfs_update_inode(), but it's possible that this was removed from btrfs_create() for performance reasons, in which case a change in how ACLs are processed (or apply the umask always regardless of ACLs) might be needed in order to create the inode with the right umask from the start.

BTW, this only affects empty files, as soon as some content makes it into the files, the file size will be changed which will make the in-memory inode dirty and will force it to be flushed to disk at some point.

I'm preparing a second patch that refactors the code above a bit in order to prevent repetition of the "drop_inode = 1; goto out_unlock;" snippet.
Comment 3 rcs 2012-11-29 18:44:03 UTC
Kernel 3.2.1-gentoo is also affected
Comment 4 rcs 2012-11-29 18:48:14 UTC
(In reply to comment #1)
> BTW, do you know of a kernel where this problem doesn't occur?
No. All kernels I tested have this bug.
Comment 5 Filipe Brandenburger 2012-11-30 04:21:30 UTC
Patch set submitted to mailing lists:
http://lkml.indiana.edu/hypermail/linux/kernel/1211.3/03273.html
Comment 6 Filipe Brandenburger 2012-11-30 04:44:22 UTC
Created attachment 87891 [details]
Script to check whether the bug is present

I wrote a simple script that reproduces the issue fairly easily and quickly and in a somewhat reliable way.

It uses the fact that this happens when creating an empty file and flushing the inode before it's changed in another way. I'm doing that by umounting the filesystem right after creating the file.

The script creates a temp file and loop mounts it so no existing Btrfs filesystem is previously needed on the machine to test it.

I tested it under three kernels and it worked fine.

1) Fedora 17 kernel:

> $ sudo ./check_bug50861.sh 
> + umask 022
> + cd /tmp/tmp.0kkZWxljlg
> + dd if=/dev/zero of=test_btrfs.img bs=65536 count=1024
> + mkfs.btrfs test_btrfs.img
> + mkdir mnt
> + mount -o loop -t btrfs test_btrfs.img mnt/
> + :
> + umount mnt/
> + mount -o loop -t btrfs test_btrfs.img mnt/
> + ls -l mnt/testfile
> -rw-rw-rw-. 1 root root 0 Nov 29 20:33 mnt/testfile
> 
> Kernel version: 3.6.6-1.fc17.x86_64
> Bug #50861: PRESENT


2) Upstream from git (at commit e23739b4ade80a3a7f87198f008f6c44a7cbc9fd):

> $ sudo ./check_bug50861.sh 
> + umask 022
> + cd /tmp/tmp.ke6IE0MqXd
> + dd if=/dev/zero of=test_btrfs.img bs=65536 count=1024
> + mkfs.btrfs test_btrfs.img
> + mkdir mnt
> + mount -o loop -t btrfs test_btrfs.img mnt/
> + :
> + umount mnt/
> + mount -o loop -t btrfs test_btrfs.img mnt/
> + ls -l mnt/testfile
> -rw-rw-rw-. 1 root root 0 Nov 29 20:35 mnt/testfile
> 
> Kernel version: 3.7.0-filbranden+
> Bug #50861: PRESENT


3) Kernel from git + the two patches submitted to the mailing list:

> $ sudo ./check_bug50861.sh 
> + umask 022
> + cd /tmp/tmp.tfF7qE6JqS
> + dd if=/dev/zero of=test_btrfs.img bs=65536 count=1024
> + mkfs.btrfs test_btrfs.img
> + mkdir mnt
> + mount -o loop -t btrfs test_btrfs.img mnt/
> + :
> + umount mnt/
> + mount -o loop -t btrfs test_btrfs.img mnt/
> + ls -l mnt/testfile
> -rw-r--r--. 1 root root 0 Nov 29 20:32 mnt/testfile
> 
> Kernel version: 3.7.0-bug50861b+
> Bug #50861: FIXED


I hope that helps.

Cheers,
Filipe
Comment 7 Filipe Brandenburger 2012-12-21 16:30:30 UTC
Hi,

The fix for this bug was committed to the official Linux git repository:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=9185aa587b7425f8f4520da2e66792f5f3c2b815

It will likely be available in Linux 3.8 and should first appear in 3.8-rc1. I'll mark this bug as FIXED as soon as 3.8-rc1 is out and I can confirm that it's fixed there.

Thanks,
Filipe
Comment 8 Filipe Brandenburger 2013-02-23 22:14:34 UTC
I just checked this in last released version 3.8.0 and I confirmed that it's fixed there.

Thanks,
Filipe


$ sudo ./check_bug50861.sh 
+ umask 022
+ cd /tmp/tmp.NzlKhdCVLW
+ dd if=/dev/zero of=test_btrfs.img bs=65536 count=1024
+ mkfs.btrfs test_btrfs.img
+ mkdir mnt
+ mount -o loop -t btrfs test_btrfs.img mnt/
+ :
+ umount mnt/
+ mount -o loop -t btrfs test_btrfs.img mnt/
+ ls -l mnt/testfile
-rw-r--r--. 1 root root 0 Feb 23 14:11 mnt/testfile

Kernel version: 3.8.0
Bug #50861: FIXED

Note You need to log in before you can comment on or make changes to this bug.