On Fri, Dec 8, 2023 at 8:53 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 07-12-23 14:38:25, Amir Goldstein wrote:
> > In preparation for pre-content permission events with file access range,
> > move fsnotify_file_perm() hook out of security_file_permission() and into
> > the callers that have the access range information and pass the access
> > range to fsnotify_file_perm().
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> So why don't you pass the range into security_file_permission() instead of
> pulling fsnotify out of the hook? I mean conceptually the accessed range
> makes sense for the hook as well although no LSM currently cares about it.
> Also it would address the Christian's concern.
>
I don't think it would be nice if the signature of
security_file_permission() did not match the signature of the LSM
methods of the same name (i.e. call_int_hook(file_permission, 0, file, mask);
I think that calling fsnotify_perm(), which is not an LSM,
from security_file_permission()/security_file_open() was a hack to
begin with and that decoupling fsnotify_perm() hooks from security
hooks is a good thing.
Also, it is needed for future work. If you look at commit
"fs: hold s_write_srcu for pre-modify permission events on write"
on my sb_write_barrier branch, you will see that the fsnotify
modify permission hook moves (again) into start_file_write_area().
So yes, fsnotify permission hooks become first class vfs hooks and
not LSM hooks, but then again, fsnotify_xxx hooks for async events
are already first class vfs hooks, and many times in the same
vfs_xxx helpers that will have the fsnotify permission hooks in them.
> > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> > index 0a9d6a8a747a..45e6ecbca057 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -103,7 +103,8 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
> > /*
> > * fsnotify_file_perm - permission hook before file access
> > */
> > -static inline int fsnotify_file_perm(struct file *file, int perm_mask)
> > +static inline int fsnotify_file_perm(struct file *file, int perm_mask,
> > + const loff_t *ppos, size_t count)
> > {
> > __u32 fsnotify_mask = FS_ACCESS_PERM;
>
> Also why do you actually pass in loff_t * instead of plain loff_t? You
> don't plan to change it, do you?
No I don't.
I used NULL to communicate "no range info" to fanotify.
It is currently only used from iterate_dir(), but filesystems may need to
use that to report other cases of pre-content access with no clear range info.
I could leave fsnotify_file_perm(file, mask) for reporting events without
range info and add fsnotify_file_area(file, mask, pos, count) for reporting
access permission with range info.
Do you think that would be better?
Thanks,
Amir.
