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

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

[Vincent Bernat]

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)

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

[Vincent Bernat]

 ❦ 15 août 2015 09:37 +0200, Vincent Bernat <bernat@luffy.cx> :

> 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.

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 <viro@zeniv.linux.org.uk>
Date:   Wed Apr 1 23:59:57 2015 -0400

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

    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>


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

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

[Vincent Bernat]

 ❦ 15 août 2015 13:17 +0200, Vincent Bernat <bernat@luffy.cx> :

>> 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.
>
> 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 <viro@zeniv.linux.org.uk>
> Date:   Wed Apr 1 23:59:57 2015 -0400
>
>     9p: switch to ->read_iter/->write_iter
>
>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
>
> 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.

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.

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

[Vincent Bernat]

 ❦ 15 août 2015 09:37 +0200, Vincent Bernat <bernat@luffy.cx> :

> 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

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

4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01 is the first bad commit
commit 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01
Author: David Howells <dhowells@redhat.com>
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)

Re: [V9fs-developer] 9p/overlayfs: read error when reading an empty file

[Dominique Martinet]

Vincent Bernat wrote on Sat, Aug 15, 2015 at 03:18:56PM +0200:
> For this bug, I was able to bisect to this commit:
> 
> 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01 is the first bad commit
> commit 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01
> Author: David Howells <dhowells@redhat.com>
> 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.

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

Re: [V9fs-developer] 9p/overlayfs: read error when reading an empty file

[Vincent Bernat]

 ❦ 17 août 2015 16:11 +0200, Dominique Martinet <dominique.martinet@cea.fr> :

>> For this bug, I was able to bisect to this commit:
>> 
>> 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01 is the first bad commit
>>
>> commit 4bacc9c9234c7c8eec44f5ed4e960d9f96fa0f01
>> Author: David Howells <dhowells@redhat.com>
>> 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.
>
> 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.

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)

Re: [V9fs-developer] 9p/overlayfs: read error when reading an empty file

[Dominique Martinet]

Vincent Bernat wrote on Sat, Oct 03, 2015:
> Did you get any feedback on this? The problem is unfortunately still
> present in 4.3-rc3.

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

Re: [V9fs-developer] 9p/overlayfs: read error when reading an empty file

[Miklos Szeredi]

On Sat, Oct 3, 2015 at 9:19 PM, Dominique Martinet
<asmadeus@codewreck.org> wrote:
> Vincent Bernat wrote on Sat, Oct 03, 2015:
>> Did you get any feedback on this? The problem is unfortunately still
>> present in 4.3-rc3.
>
> 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.

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

Re: [V9fs-developer] 9p/overlayfs: what inodes/dentries are safe to use?

[Dominique Martinet]

Miklos Szeredi wrote on Mon, Oct 12, 2015:
> 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.

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

Re: [V9fs-developer] 9p/overlayfs: what inodes/dentries are safe to use?

[Miklos Szeredi]

On Mon, Oct 12, 2015 at 7:47 PM, Dominique Martinet
<asmadeus@codewreck.org> wrote:
> Miklos Szeredi wrote on Mon, Oct 12, 2015:
>> 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.
>
> 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 ?)

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

Solution to this particular problem?  Not sure.

Al?

Thanks,
Miklos

Re: [V9fs-developer] 9p/overlayfs: what inodes/dentries are safe to use?

[Eric Van Hensbergen]

On Mon, Oct 12, 2015 at 11:33 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Mon, Oct 12, 2015 at 7:47 PM, Dominique Martinet
> <asmadeus@codewreck.org> wrote:
>> Miklos Szeredi wrote on Mon, Oct 12, 2015:
>>> 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.
>>
>> 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 ?)
>
> file->f_path shouldn't be trusted by filesystems for non-directories.  Ever.
>
> Solution to this particular problem?  Not sure.
>

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