On Fri, Nov 29, 2024 at 8:16 AM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Thu, 28 Nov 2024 at 17:44, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > This sounds good, but do the watchers actually need this information > > or is it redundant to parent_id? > > Everything but mnt_id is redundant, since they can be retrieved with statmount. > > I thought, why not use the existing infrastructure in the event? But > it's not strictly needed. > > > If we are not sure that reporting fd/fid is needed, then we can limit > > FAN_REPORT_MNTID | FAN_REPORT_*FID now and consider adding it later. > > > > WDYT? > > Sounds good. > > > > You missed fanotify_should_merge(). IMO FAN_MNT_ events should never be merged > > so not sure that mixing this data in the hash is needed. > > Okay. > > > I think if we do not HAVE TO mix mntid info and fid info, then we better > > stick with event->fd + mntid and add those fields to fanotify_path_event. > > Okay. > > > See patch "fanotify: don't skip extra event info if no info_mode is set" > > in Jan's fsnotify_hsm branch. > > This should be inside copy_info_records_to_user(). > > Makes sense. I was wondering why copy_info_records_to_user() was > called conditionally. Up to pre-content events and FAN_EVENT_INFO_TYPE_RANGE, fanotify_event_metadata had preserved its legacy non-extended format, unless userspace explicitly opted-in to acknowledge the format extension with one of the FAN_REPORT_ init flags. If you take the route of FAN_REPORT_MNTID that I suggested, and add this flag to FANOTIFY_INFO_MODES then copy_info_records_to_user() will be called anyway. This brings the question: should the FAN_REPORT_MNT flag be added to FANOTIFY_ADMIN_INIT_FLAGS or FANOTIFY_USER_INIT_FLAGS? Currently, watching a sb/mount requires capable(SYS_ADMIN), but I have a pretty simple patchset [1] to require ns_capable(SYS_ADMIN). Thing is, I never got feedback from userspace that this is needed [2]. Seeing that statmount/listmount() requires at most ns_capable(SYS_ADMIN), I am guessing that you would also want mount monitor to require at most ns_capable(SYS_ADMIN) rather than capable(SYS_ADMIN)? If that is the case, feel free to pick up my patches and test mount monitor inside userns. Which then still leaves the question: do we want to allow an unprivileged user to setup an inode mark with FAN_MNT_ events on a mount point? This can be a bit tricky, because currently setting up an inode mark only requires that the caller has access to that path. That could lead to sending events with mntid from other namespaces to unprivileged user watching a mount point inode. Option #1: do not allow setting FAN_MNT_ events on inode marks (for now) Option #2: apply the same requirement for sb mark from fanotify_userns patch to inode mark on group with FAN_REPORT_MNTID. Thanks, Amir. [1] https://github.com/amir73il/linux/commits/fanotify_userns/ [2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxiwGTg=FeO6iiLEwtsP9eTudw-rsLD_0u3NtG8rz5chFg@xxxxxxxxxxxxxx/
