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: > > > > 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. > > 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 > > > ima_file_mprotect > > > 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 > > > print_bad_pte > > > file_path > > > seq_print_user_ip > > > __mnt_want_write_file > > > __mnt_drop_write_file > > > file_dentry_name > > > > > > 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. > > > > 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. Thanks, Miklos
