On Tue, Jul 8, 2025 at 1:40 PM Jan Kara <jack@xxxxxxx> wrote: > > 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? Not sure what complication you are referring to. Maybe this would have been more clear: + if ((flags & FAN_REPORT_RESPONSE_ID) && (!fid_mode || + ((flag & FAN_REPORT_FD_ERROR) || class == FAN_CLASS_NOTIFY)) + return -EINVAL; + Yes, !fid_mode is the more important check. The checks in the second line are because the combination of FAN_REPORT_RESPONSE_ID with those other flags does not make sense. FAN_CLASS_NOTIFY does not support permission events and when not reporting event->fd reporting FD_ERROR does not make sense. My intention is to reduce the test matrix for flag combinations that do not make sense. Thanks, Amir.
