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: > > > > > > Hi! > > > > > > 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 reposonse_id behavior will require opt-in with init flag FAN_REPORT_RESPONSE_ID. I suggest to change the uapi as follows: @@ -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 With FAN_CLASS_PRE_CONTENT_FID mode, the value reported in event->id can ALWAYS be written to response->id, regardless of FAN_REPORT_RESPONSE_ID because it can never be an error value. Thanks, Amir.
