On Tue, Nov 12, 2024 at 8:54 PM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@xxxxxxxxxxxxxx> wrote: > > > > + /* > > + * This permission hook is different than fsnotify_open_perm() hook. > > + * This is a pre-content hook that is called without sb_writers held > > + * and after the file was truncated. > > + */ > > + return fsnotify_file_area_perm(file, MAY_OPEN, &file->f_pos, 0); > > } > > I still object to this all. > > You can't say "permission denied" after you've already truncated the > file. It's not a sane model. I complained about that earlier, it seems > that complaint was missed in the other complaints. > Not missed. I answered here: https://lore.kernel.org/linux-fsdevel/CAOQ4uxg0k4bGz6zOKS+Qt5BjEqDdUhvgG+5pLBPqSCcnQdffig@xxxxxxxxxxxxxx/ Starting with "...I understand why it seems stupid..." Nevertheless, we can also drop this patch for now, I don't think the post-open is a must-have hook for HSM. > Also, this whole "This permission hook is different than > fsnotify_open_perm() hook" statement is purely because > fsnotify_open_perm() itself was broken and called from the wrong place > as mentioned in the other email. > You wrote it should be called "in the open path" - that is ambiguous. pre-content hook must be called without sb_writers held, so current (in linux-next) location of fsnotify_open_perm() is not good in case of O_CREATE flag, so I am not sure where a good location is. Easier is to drop this patch. > Fix *THAT* first, then unify the two places that should *not* be > different into one single "this is the fsnotify_open" code. And that > place explicitly sets that FMODE_NOTIFY_PERM bit, and makes sure that > it does *not* set it for FMODE_NONOTIFY or FMODE_PATH cases. > > And then please work on making sure that that isn't called unless > actually required. > > The actual real "pre-content permission events" should then ONLY test > the FMODE_NOTIFY_PERM bit. Nothing else. None of this "re-use the > existing fsnotify_file() logic" stuff. Noe extra tests, no extra > logic. > > Don't make me jump through filve layers of inline functions that all > test different 'mask' bits, just to verify that the open / read / > write paths don't do something stupid. > > IOW, make it straightforward and obvious what you are doing, and make > it very clear that you're not pointlessly testing things like > FMODE_NONOTIFY when the *ONLY* thing that should be tested is whether > FMODE_NOTIFY_PERM is set. > > Please. Yes. Clear. Thanks for taking the time to look closer. and thanks for the feedback, Amir.
