On Mon 07-07-25 18:33:41, Amir Goldstein wrote: > On Tue, Jul 1, 2025 at 9:23 AM Ibrahim Jirdeh <ibrahimjirdeh@xxxxxxxx> wrote: > > On 6/30/25, 9:06 AM, "Amir Goldstein" <amir73il@xxxxxxxxx <mailto:amir73il@xxxxxxxxx>> wrote: > > > On Mon, Jun 30, 2025 at 4:50 PM Jan Kara <jack@xxxxxxx> wrote: > > > > I agree expanding fanotify_event_metadata painful. After all that's the > > > > reason why we've invented the additional info records in the first place :). > > > > So I agree with putting the id either in a separate info record or overload > > > > something in fanotify_event_metadata. > > > > > > > > On Sun 29-06-25 08:50:05, Amir Goldstein wrote: > > > > > I may have mentioned this before, but I'll bring it up again. > > > > > If we want to overload event->fd with response id I would consider > > > > > allocating response_id with idr_alloc_cyclic() that starts at 256 > > > > > and then set event->fd = -response_id. > > > > > We want to skip the range -1..-255 because it is used to report > > > > > fd open errors with FAN_REPORT_FD_ERROR. > > > > > > > > I kind of like this. It looks elegant. The only reason I'm hesitating is > > > > that as you mentioned with persistent notifications we'll likely need > > > > 64-bit type for identifying event. But OTOH requirements there are unclear > > > > and I can imagine even userspace assigning the ID. In the worst case we > > > > could add info record for this persistent event id. > > > > > > Yes, those persistent id's are inherently different from the response key, > > > so I am not really worried about duplicity. > > > > > > > So ok, let's do it as you suggest. > > > > > > Cool. > > > > > > I don't think that we even need an explicit FAN_REPORT_EVENT_ID, > > > because it is enough to say that (fid_mode != 0) always means that > > > event->fd cannot be >= 0 (like it does today), but with pre-content events > > > event->fd can be a key < -255? > > > > > > Ibrahim, > > > > > > Feel free to post the patches from my branch, if you want > > > post the event->fd = -response_id implementation. > > > > > > I also plan to post them myself when I complete the pre-dir-content patches. > > > > Sounds good. I will pull in the FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID branch > > and resubmit this patch now that we have consensus on the approach here. > > FYI, I pushed some semantic changed to fan_pre_content_fid branch: > > - Created shortcut macro FAN_CLASS_PRE_CONTENT_FID > - Created a group priority FSNOTIFY_PRIO_PRE_CONTENT_FID > > Regarding the question whether reporting response_id instead of event->fd > requires an opt-in, so far my pre-dir-content patches can report event->fd, > so my preference id the response_id behavior will require opt-in with init > flag FAN_REPORT_RESPONSE_ID. No problem with the FAN_REPORT_RESPONSE_ID feature flag, just we'll see whether the classical fd-based events are useful enough with pre-dir-content events to justify their existence ;). > @@ -67,6 +67,7 @@ > #define FAN_REPORT_TARGET_FID 0x00001000 /* Report dirent target id */ > #define FAN_REPORT_FD_ERROR 0x00002000 /* event->fd can report error */ > #define FAN_REPORT_MNT 0x00004000 /* Report mount events */ > +#define FAN_REPORT_RESPONSE_ID 0x00008000 /* event->fd is a response id */ > > /* Convenience macro - FAN_REPORT_NAME requires FAN_REPORT_DIR_FID */ > #define FAN_REPORT_DFID_NAME (FAN_REPORT_DIR_FID | FAN_REPORT_NAME) > @@ -144,7 +145,10 @@ struct fanotify_event_metadata { > __u8 reserved; > __u16 metadata_len; > __aligned_u64 mask; > - __s32 fd; > + union { > + __s32 fd; > + __s32 id; /* FAN_REPORT_RESPONSE_ID */ > + } > __s32 pid; > }; > > @@ -228,7 +232,10 @@ struct fanotify_event_info_mnt { > #define FAN_RESPONSE_INFO_AUDIT_RULE 1 > > struct fanotify_response { > - __s32 fd; > + union { > + __s32 fd; > + __s32 id; /* FAN_REPORT_RESPONSE_ID */ > + } > __u32 response; > }; > > And to add a check like this: > > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1583,6 +1583,16 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, > flags, unsigned int, event_f_flags) > (class | fid_mode) != FAN_CLASS_PRE_CONTENT_FID) > return -EINVAL; > > + /* > + * With group that reports fid info and allows only pre-content events, > + * user may request to get a response id instead of event->fd. > + * FAN_REPORT_FD_ERROR does not make sense in this case. > + */ > + if ((flags & FAN_REPORT_RESPONSE_ID) && > + ((flag & FAN_REPORT_FD_ERROR) || > + !fid_mode || class != FAN_CLASS_PRE_CONTENT_FID)) > + return -EINVAL; > + > > > This new group mode is safe, because: > 1. event->fd is redundant to target fid > 2. other group priorities allow mixing async events in the same group > async event can have negative event->fd which signifies an error > to open event->fd I'm not sure I follow here. I'd expect: if ((flags & FAN_REPORT_RESPONSE_ID) && !fid_mode) return -EINVAL; I.e., if you ask for event ids, we don't return fds at all so you better had FID enabled to see where the event happened. And then there's no need for FAN_CLASS_PRE_CONTENT_FID at all. Yes, you cannot mix async fanotify events with fd with permission events using event id but is that a sane combination? What case do you have in mind that justifies this complication? Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
