On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote: > This patch adds a flag, FAN_INFO and an extensible buffer to provide > additional information about response decisions. The buffer contains > one or more headers defining the information type and the length of the > following information. The patch defines one additional information > type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will > allow for the creation of other information types in the future if other > users of the API identify different needs. > > Suggested-by: Steve Grubb <sgrubb@xxxxxxxxxx> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 > Suggested-by: Jan Kara <jack@xxxxxxx> > Link: https://lore.kernel.org/r/20201001101219.GE17860@xxxxxxxxxxxxxx > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx> Thanks for the patches. They look very good to me. Just two nits below. I can do the small updates on commit if there would be no other changes. But I'd like to get some review from audit guys for patch 3/3 before I commit this. > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index caa1211bac8c..cf3584351e00 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path, > return client_fd; > } > > +static int process_access_response_info(int fd, const char __user *info, size_t info_len, > + struct fanotify_response_info_audit_rule *friar) I prefer to keep lines within 80 columns, unless there is really good reason (like with strings) to have them longer. BTW, why do you call the info structure 'friar'? I feel some language twist escapes me ;) > +{ > + if (fd == FAN_NOFD) > + return -ENOENT; I would not test 'fd' in this function at all. After all it is not part of the response info structure and you do check it in process_access_response() anyway. > + > + 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. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
