9p/overlayfs: read error when reading an empty file

Hi!

I have found a regression which was introduced after 4.0 in 9p/overlayfs. This regression happens when the lower directory is a 9p mount, the upperdir is an empty tmpfs and we try to read 0 bytes from an empty file (something than gcc is doing when trying to read an include).

The following program can be used to trigger the problem:

#v+
#include <assert.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>

int main(int argc, const char **argv)

{
    assert(argc == 2);
    char buffer[256];
    int fd = open(argv[1], O_RDONLY|O_NOCTTY);
    assert(fd >= 0);
    assert(read(fd, buffer, 0) == 0);
    return 0;
}

#v-

read() returns -30720.

This works fine with a 4.0 kernel and breaks with a 4.1 kernel.

However, I am unable to tell if this is fixed in more recent kernels because I run into another bug. In 4.2-rc6, I am unable to do a switch root on the same setup (9p lower dir, tmpfs upper dir). I get this:

[    0.566541] BUG: unable to handle kernel paging request at 000000040020006c
[    0.570248] IP: [<ffffffffa004fe4b>] v9fs_fid_find+0x5b/0x90 [9p]
[    0.570248] PGD 64ee067 PUD 0
[    0.570248] Oops: 0000 [#1] SMP
[    0.570248] Modules linked in: overlay virtio_pci 9p fscache 9pnet_virtio virtio_ring virtio 9pnet
[    0.570248] CPU: 0 PID: 1 Comm: switch_root Not tainted 4.2.0-rc6-amd64 #1 Debian 4.2~rc6-1~exp1
[    0.570248] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[    0.570248] task: ffff88000710cc00 ti: ffff880007128000 task.ti: ffff880007128000
[    0.570248] RIP: 0010:[<ffffffffa004fe4b>]  [<ffffffffa004fe4b>] v9fs_fid_find+0x5b/0x90 [9p]
[    0.570248] RSP: 0018:ffff88000712ba70  EFLAGS: 00010246
[    0.570248] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: 0000000400200048
[    0.570248] RDX: 0000000000000001 RSI: 00000000ffffffff RDI: ffff88000688feb0
[    0.570248] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000001
[    0.570248] R10: 000000000001ca30 R11: f000000000000000 R12: ffff88000688fe58
[    0.570248] R13: ffff88000688feb0 R14: ffff88000641a748 R15: ffff88000688e618
[    0.570248] FS:  00007fcd46a88700(0000) GS:ffff880007c00000(0000) knlGS:0000000000000000
[    0.570248] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[    0.570248] CR2: 000000040020006c CR3: 000000000649a000 CR4: 00000000000006f0
[    0.570248] Stack:
[    0.570248]  ffff880006473000 00000000ffffffff 0000000000000000 ffff88000641a340
[    0.570248]  00000000000000e0 ffffffffa004ffc6 0000000000000000 ffff88000688e618
[    0.570248]  ffff880000c02048 0000000000000000 ffff880000014b00 ffff880006446800
[    0.570248] Call Trace:
[    0.570248]  [<ffffffffa004ffc6>] ? v9fs_fid_lookup_with_uid+0xf6/0x2d0 [9p]
[    0.570248]  [<ffffffffa0050209>] ? v9fs_fid_lookup+0x69/0x70 [9p]
[    0.570248]  [<ffffffffa005021e>] ? v9fs_fid_clone+0xe/0x30 [9p]
[    0.570248]  [<ffffffffa004e773>] ? v9fs_file_open+0xb3/0x190 [9p]
[    0.570248]  [<ffffffffa004e6c0>] ? v9fs_vfs_readpage+0x20/0x20 [9p]
[    0.570248]  [<ffffffff811b4596>] ? do_dentry_open+0x1c6/0x2e0
[    0.570248]  [<ffffffff811c3243>] ? path_openat+0x1d3/0x14a0
[    0.570248]  [<ffffffff81173d27>] ? follow_page_pte+0x267/0x360
[    0.570248]  [<ffffffff811c5e05>] ? do_filp_open+0x75/0xd0
[    0.570248]  [<ffffffff811bc736>] ? do_open_execat+0x66/0x150
[    0.570248]  [<ffffffff811bc84a>] ? open_exec+0x2a/0x50
[    0.570248]  [<ffffffff812069de>] ? load_script+0x1de/0x230
[    0.570248]  [<ffffffff811bc0ee>] ? copy_strings.isra.21+0x27e/0x2d0
[    0.570248]  [<ffffffff811bc3a3>] ? search_binary_handler+0x93/0x1b0
[    0.570248]  [<ffffffff811bd884>] ? do_execveat_common.isra.32+0x544/0x6c0
[    0.570248]  [<ffffffff811cbf9a>] ? dput+0x2a/0x220
[    0.570248]  [<ffffffff811bdc85>] ? SyS_execve+0x35/0x40
[    0.570248]  [<ffffffff8154c465>] ? stub_execve+0x5/0x5
[    0.570248]  [<ffffffff8154c132>] ? system_call_fast_compare_end+0xc/0x6b
[    0.570248] Code: 89 ef e8 39 bf 4f e1 49 8b 44 24 78 48 8d 48 c0 48 85 c0 b8 00 00 00 00 48 0f 44 c8 eb 04 48 83 e9 40 48 85 c9 74 12 85 ed 75 0e <3b> 59 24 74 09 48 8b 49 40 48 85 c9 75 e5 4c 89 ef c6 07 00 0f
[    0.570248] RIP  [<ffffffffa004fe4b>] v9fs_fid_find+0x5b/0x90 [9p]
[    0.570248]  RSP <ffff88000712ba70>
[    0.570248] CR2: 000000040020006c
[    0.690182] ---[ end trace 40caffb61d0461c6 ]---
[    0.693741] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
[    0.693741]
[    0.696018] Kernel Offset: disabled

Trying to bisect this one, I end up into another bug when mounting a 9p filesystem:

# mount -t 9p configshare /tmp/config -o trans=virtio,version=9p2000.u,access=0,rw
mount: mounting configshare on /tmp/config failed: Invalid argument

However, this one has been fixed between v4.1 and v4.2-rc6. It just makes git bisecting difficult.

I am using a shell script to setup all that:
https://github.com/vincentbernat/eudyptula-boot

The gist is something like that:
mkdir /tmp/target
mkdir /tmp/target/ro
mkdir /tmp/target/overlay
mkdir /tmp/target/rw
mount -n -t 9p rootshare /tmp/target/ro -o trans=virtio,version=9p2000.u,ro
mount -n -t tmpfs tmpfs /tmp/target/rw -o rw
mkdir /tmp/target/rw/workdir
mkdir /tmp/target/rw/upperdir
mount -n -t $1 overlayfs /tmp/target/overlay \
-o lowerdir=/tmp/target/ro,upperdir=/tmp/target/rw/upperdir,workdir=/tmp/target/rw/workdir,noatime
mkdir /tmp/target/overlay/tmp/config
mount --bind /tmp/config /tmp/target/overlay/tmp/config

cp /init /tmp/target/overlay/tmp
exec switch_root /tmp/target/overlay /tmp/init

I am currently in the process to do a git bisect between 4.0 and 4.1 to find the commit that have introduced the first bug, but any hint is welcome.

Use variable names that mean something.
            - The Elements of Programming Style (Kernighan & Plauger)

❦ 15 août 2015 09:37 +0200, Vincent Bernat <[email protected]> :

It took me some time to bissect this one because I also run into an infinite loop caused by 070b36 and fixed by 8e3c50. Finally, the culprit for the above bug seems to be:

commit e494b6b5e1034db00571c44e089e6fe3845b6e8c

Author: Al Viro <[email protected]>
Date:   Wed Apr 1 23:59:57 2015 -0400

9p: switch to ->read_iter/->write_iter

This commit + 8e3c50 triggers the bug. This commit~1 + 8e3c50 doesn't. Unfortunately, it is far too extensive to try to revert it on top of 4.1.

When in doubt, tell the truth.
		-- Mark Twain

❦ 15 août 2015 13:17 +0200, Vincent Bernat <[email protected]> :

After some more testing, I discovered that this bug also happens without overlayfs (despite what I said in the first post). The fix is in fact pretty easy (ret should be initialized to 0 in v9fs_file_read_iter). I am sending a proper patch in a minute.

ROMEO:		Courage, man; the hurt cannot be much.
MERCUTIO:	No, 'tis not so deep as a well, nor so wide
			as a church-door; but 'tis enough, 'twill serve.

❦ 15 août 2015 09:37 +0200, Vincent Bernat <[email protected]> :

For this bug, I was able to bisect to this commit:

4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01 is the first bad commit commit 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01

Author: David Howells <[email protected]>
Date:   Thu Jun 18 14:32:31 2015 +0100

overlayfs: Make f_path always point to the overlay and f_inode to the underlay

In fact, any file reading of files present in the lower layer will lead to this. No need to try to switch root or anything complex.

Use variable names that mean something.
            - The Elements of Programming Style (Kernighan & Plauger)

Vincent Bernat wrote on Sat, Aug 15, 2015 at 03:18:56PM +0200:

Hmm, that is going to be annoying to fix, since we use file->f_path.dentry to get a new fid to the same file (clone), and current code pretty much depends on that...

I guess it used to work because file->f_op, file->f_inode and file->f_path used to all be from the same layer, which isn't the case anymore.

Does any overlayfs person reading this know if it's still possible to get the right dentry at this point?
I'm sure the object exists somewhere in memory, but it could be annoying to find.

Thanks,

Dominique

❦ 17 août 2015 16:11 +0200, Dominique Martinet <[email protected]> :

Hi Dominique!

Did you get any feedback on this? The problem is unfortunately still present in 4.3-rc3.

Let the machine do the dirty work.
            - The Elements of Programming Style (Kernighan & Plauger)

Vincent Bernat wrote on Sat, Oct 03, 2015:

Sorry, haven't got any reply but I bet it is :(

I've been meaning to ask again after a while but it slipped out of my head again...

Basically 9P stores informations in both the inode and dentry, the change (4bacc9 "overlayfs: Make f_path always point to the overlay and f_inode to the underlay") makes it so that we get unexpected info in f_path->dentry->d_sb->s_fs_info and crash...

I actually am not quite sure where the change needed belongs in the first place, I don't even know how the 9P layer could check if the filep isn't a "9p filep" in the first place, nor how it could work around it if it notices that's what happened, so I'm afraid I won't be of much help there...

We probably can remove the need for dentry->d_sb->s_fs_info in this case (it's the 9p session info which we can get from the inode, which is also available) but 9p keeps a list with dentries for fids that I have no idea if it will make sense and/or be safe if we start mixing up overlay stuff in.
Also, and worse, this only fixes the case where 9p is the underlay -- if someone gets the idea to use 9p as overlay then we'll get a "bad" inode and good dentry which will cause different problems, so once again we need to know where stuff comes from.
And what about stacked overlays when 9P is in the middle, neither inode nor file would be "our's" ?...

I'm sure I'm missing something simple and overcomplicating things, but I just don't have time to try to dig into the overlayfs code right now sorry.

Dominique

The solution depends on how 9p handles hard links. If any dentry will do then d_find_any_alias() will get you a dentry (any dentry) from the inode.

Otherwise I know of no mechanism to get the backing dentry from the file.

Thanks,
Miklos

Miklos Szeredi wrote on Mon, Oct 12, 2015:

I've got two problems at the moment:
- first, 9P stores informations in the superblock (accessed
through either dentry->d_sb->s_fs_info or inode->i_sb->...), and operations such as read will get passed an inode and a file (which has a dentry), but 9P doesn't know which to trust (if I go this right - if 9P is underlay then the inode is 9p's, if 9P is overlay then the dentry is ?)
So we probably need a wrapper to get this right, somehow, and use it everywhere there's a chance we get an alien inode/dentry.

- The second problem is more inherent to the 9p protocol, that is every
access is made through a "fid" and the client navigates through the filesystem tree one operation at a time ("walk") - walk will either "clone" a fid or get one from its parent directory (it can do more complex lookups but the kernel client never does)

At some point we need to open a new fid to a currently open file. The current algorithm uses dentry->d_fsdata as a list of fids, so if we already have a fid pointing to the dentry we can just usethat one, or if there's nothing we'll either use the parent or start from scratch, but the point is that once again I don't know if I can "trust" dentry->d_fsdata to be a valid pointer or not.

Safest bet would be to remove this mechanism altogether, but that's going to have a noticiable performance hit as we do this kind of clone fairly often (9p needs more 'fids' than we have 'dentries' because some operations will mutate or close them, so we clone pretty often) Otherwise if there's some kind of flag we can check to tell if THAT dentry is ours then we could only do this kind of lookup when that is set, but I don't think there is?

Thanks,

Dominique Martinet

file->f_path shouldn't be trusted by filesystems for non-directories. Ever.

Solution to this particular problem? Not sure.

Al?

Thanks,
Miklos

It ends up being a very difficult problem. At the heart of things it has to do with no direct 1:1 mapping between 9P and VFS for the concept of a fid, because a fid represents both open files and transient pointers to locations in the file hierarchy. To complicate things, fids are technically per-user and have a good deal of state associated with them including security context, position in a multi-packet directory listing, etc. To try and cope, I went with a mix of attaching fids to inodes, dentries, and file pointers. The original concept was that file pointers were only for open files and dentries were for transient location pointers. The attachment to inodes was originally for housekeeping, but more recently I was looking at moving completely from dentries to inode based attachment to try and solve the open-unlink-stat bug on the client side versus making the server deal with it.

So, any dentry won't do, but we could try and traverse the fids attached to an inode and guess which one is the right one by looking at the ownership information and state within the fid. This would advocate hanging everything off of the inode instead of the dentry/file. Of course, I'd have to reliably get to 9P's inode -- but I'm assuming by the time I get to it through whatever layers above it, I should be able to resolve my inode. It means a slightly worse access time if you have lots of fids associated with an inode, but maybe we can make that traversal more efficient if it actually materializes as a performance constraint. Its fairly heavy surgery, but may solve some other problems -- of course, I'm worried it might create other issues.

We can try a bit of a more complicated solution by being a bit more paranoid about VFS structures and always keying the 9P structures. If private data doesn't match the 9P key then we fallback to some long latency path to recover the required information....

thoughts?

-eric