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@xxxxxxxx>
SUSE Labs, CR
