Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context

On 2023-01-03 13:42, Jan Kara wrote: > On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote: > > > > + > > > > + if (info_len != sizeof(*friar)) > > > > + return -EINVAL; > > > > + > > > > + if (copy_from_user(friar, info, sizeof(*friar))) > > > > + return -EFAULT; > > > > + > > > > + if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE) > > > > + return -EINVAL; > > > > + if (friar->hdr.pad != 0) > > > > + return -EINVAL; > > > > + if (friar->hdr.len != sizeof(*friar)) > > > > + return -EINVAL; > > > > + > > > > + return info_len; > > > > +} > > > > + > > > > > > ... > > > > > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group, > > > > return -EINVAL; > > > > } > > > > > > > > - if (fd < 0) > > > > + if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT)) > > > > return -EINVAL; > > > > > > > > - if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT)) > > > > + if (response & FAN_INFO) { > > > > + ret = process_access_response_info(fd, info, info_len, &friar); > > > > + if (ret < 0) > > > > + return ret; > > > > + } else { > > > > + ret = 0; > > > > + } > > > > + > > > > + if (fd < 0) > > > > return -EINVAL; > > > > > > And here I'd do: > > > > > > if (fd == FAN_NOFD) > > > return 0; > > > if (fd < 0) > > > return -EINVAL; > > > > > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify > > > extra info is understood by the kernel so that application writing fanotify > > > responses has a way to check which information it can provide to the > > > kernel. > > > > The reason for including it in process_access_response_info() is to make > > sure that it is included in the FAN_INFO case to detect this extension. > > If it were included here > > I see what you're getting at now. So the condition > > if (fd == FAN_NOFD) > return 0; > > needs to be moved into > > if (response & FAN_INFO) > > branch after process_access_response_info(). I still prefer to keep it > outside of the process_access_response_info() function itself as it looks > more logical to me. Does it address your concerns? Ok. Note that this does not return zero to userspace, since this function's return value is added to the size of the struct fanotify_response when there is no error.

For that reason, I think it makes more sense to return -ENOENT, or some other unused error code that fits, unless you think it is acceptable to return sizeof(struct fanotify_response) when FAN_INFO is set to indicate this.