On Fri, Jun 9, 2023 at 6:00 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Fri, 9 Jun 2023 at 16:42, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Fri, Jun 9, 2023 at 5:28 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > On Fri, Jun 9, 2023 at 4:15 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > > > > > > > On Fri, 9 Jun 2023 at 09:32, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > > > > > Miklos, > > > > > > > > > > This is the solution that we discussed for removing FMODE_NONOTIFY > > > > > from overlayfs real files. > > > > > > > > > > My branch [1] has an extra patch for remove FMODE_NONOTIFY, but > > > > > I am still testing the ovl-fsnotify interaction, so we can defer > > > > > that step to later. > > > > > > > > > > I wanted to post this series earlier to give more time for fsdevel > > > > > feedback and if these patches get your blessing and the blessing of > > > > > vfs maintainers, it is probably better that they will go through the > > > > > vfs tree. > > > > > > > > > > I've tested that overlay "fake" path are still shown in /proc/self/maps > > > > > and in the /proc/self/exe and /proc/self/map_files/ symlinks. > > > > > > > > > > The audit and tomoyo use of file_fake_path() is not tested > > > > > (CC maintainers), but they both look like user displayed paths, > > > > > so I assumed they's want to preserve the existing behavior > > > > > (i.e. displaying the fake overlayfs path). > > > > > > > > I did an audit of all ->vm_file and found a couple of missing ones: > > > Hi Miklos, Trying to get back to this. > > > Wait, but why only ->vm_file? > > Because we don't get to intercept vm_ops, so anything done through > mmaps will not go though overlayfs. That would result in apparmor > missing these, for example. > As discussed, unless we split ovl realfile to 3 variants: lower/upper/mmap vm_file will be a backing_file and so will the read/write realfile, sometimes the low level helpers need the real path and sometimes then need the fake path. > > > We were under the assumption the fake path is only needed > > > for mapped files, but the list below suggests that it matters > > > to other file operations as well... > > > > > > > > > > > dump_common_audit_data This is an example of a generic helper. There is no way of knowing if it really wants the real or fake path. It depends if the audit rule was set on ovl path or on real path, but if audit rule was set on real path, we should not let the fake path avert the audit, same as we should not have let the real file avert fsnotify events. It seems to me the audit rules are set on inodes and compare st_dev/st_ino, so it is more likely that for operations on the realfile, the real path makes more sense to print. > > > > ima_file_mprotect >From the little experience I have with overlayfs and IMA, nothing works much wrt measuring and verifying operations on the real files. > > > > common_file_perm (I don't understand the code enough to know whether > > > > it needs fake dentry or not) > > > > aa_file_perm > > > > __file_path_perm Same rationale as in audit. We do not want to avert permission hooks for rules that were set on the real inode/path, so it makes way more sense to use real path in these common helpers. Printing the wrong path is not as bad as not printing an audit! > > > > print_bad_pte I am not very concerned about printing real path in kmsg errors. Those errors are more likely cause by underlying fs/block issues anyway? > > > > file_path Too low level. Call sites that need to print the fake path can use d_path() > > > > seq_print_user_ip Yes. > > > > __mnt_want_write_file > > > > __mnt_drop_write_file > > > > file_dentry_name Too low level. > > > > > > > > Didn't go into drivers/ and didn't follow indirect calls (e.g. > > > > f_op->fsysnc). I also may have missed something along the way, but my > > > > guess is that I did catch most cases. > > > > > > Wow. So much for 3-4 special cases... > > > > > > Confused by some of the above. > > > > > > Why would we want __mnt_want_write_file on the fake path? > > > We'd already taken __mnt_want_write on overlay and with > > > real file we need __mnt_want_write on the real path. > > It's for write faults on memory maps. The code already branches on > file->f_mode, I don't think it would be a big performance hit to check > FMODE_FAKE_PATH. > Again, FMODE_BACKING is set on the same realfile that is used for read/write_iter. Unless you will be willing to allocate two realfiles. I added a patch to take mnt_writers refcount for both fake and real path for writable file. Page faults only need to take freeze protection on real sb. no? > > > > > > For IMA/LSMs, I'd imagine that like fanotify, they would rather get > > > the real path where the real policy is stored. > > > If some log files end with relative path instead of full fake path > > > it's probably not the worst outcome. > > > > > > Thoughts? > > > > Considering the results of your audit, I think I prefer to keep > > f_path fake and store real_path in struct file_fake for code > > that wants the real path. > > > > This will keep all logic unchanged, which is better for my health. > > and only fsnotify (for now) will start using f_real_path() to > > generate events on real fs objects. > > That's also an option. > > I think f_fake_path() would still be a move in the right direction. > We have 46 instances of file_dentry() currently and of those special > cases most are cosmetic, while missing file_dentry() ones are > crashable. > Here is what I have so far: https://github.com/amir73il/linux/commits/ovl_fake_path I tested it with fstests and nothing popped up. If this looks like a good start to you, I can throw it at linux-next and see and anything floats. Thanks, Amir.
