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

On Mon 16-01-23 15:42:29, Richard Guy Briggs wrote: > 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. Right, good point. 0 is not a good return value in this case. > 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. Yeah, my intention was to indicate "success" to userspace so I'd like to return whatever we return for the case when struct fanotify_response is accepted for a normal file descriptor - looks like info_len is the right value. Thanks!

Honza -- Jan Kara <jack(a)suse.com&gt;