On Tue, Jul 8, 2025 at 3:43 PM Jan Kara <jack@xxxxxxx> wrote: > > On Tue 08-07-25 14:58:31, Amir Goldstein wrote: > > On Tue, Jul 8, 2025 at 1:40 PM Jan Kara <jack@xxxxxxx> wrote: > > > > --- 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; > > + > > Right, I can understand this easily :) Thanks for clarification. > > > 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. > > But FAN_REPORT_FD_ERROR influences also the behavior of pidfd (whether we > report full errno there or only FAN_NOPIDFD / FAN_EPIDFD). Hence I think > the exclusion with FAN_REPORT_FD_ERROR is wrong. I keep forgetting about this one :) Yeh, better leave it out then. That should be enough: + if ((flags & FAN_REPORT_RESPONSE_ID) && + (!fid_mode || class == FAN_CLASS_NOTIFY)) + return -EINVAL; Thanks, Amir.
