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

[Amir Goldstein <amir73il@gmail.com>]

On Tue, May 17, 2022 at 2:31 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > >
> > > > This patch adds 2 structure members to the response returned from user
> > > > space on a permission event. The first field is 32 bits for the context
> > > > type.  The context type will describe what the meaning is of the second
> > > > field. The default is none. The patch defines one additional context
> > > > type which means that the second field is a union containing a 32-bit
> > > > rule number. This will allow for the creation of other context types in
> > > > the future if other users of the API identify different needs.  The
> > > > second field size is defined by the context type and can be used to pass
> > > > along the data described by the context.
> > > >
> > > > To support this, there is a macro for user space to check that the data
> > > > being sent is valid. Of course, without this check, anything that
> > > > overflows the bit field will trigger an EINVAL based on the use of
> > > > FAN_INVALID_RESPONSE_MASK in process_access_response().
> > > >
> > > > Suggested-by: Steve Grubb <sgrubb@redhat.com>
> > > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > > > Suggested-by: Jan Kara <jack@suse.cz>
> > > > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > ...
> > > >  static int process_access_response(struct fsnotify_group *group,
> > > > -                                  struct fanotify_response *response_struct)
> > > > +                                  struct fanotify_response *response_struct,
> > > > +                                  size_t count)
> > > >  {
> > > >         struct fanotify_perm_event *event;
> > > >         int fd = response_struct->fd;
> > > >         u32 response = response_struct->response;
> > > >
> > > > -       pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > > -                fd, response);
> > > > +       pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > > +                group, fd, response, response_struct->extra_info_type, count);
> > > > +       if (fd < 0)
> > > > +               return -EINVAL;
> > > >         /*
> > > >          * make sure the response is valid, if invalid we do nothing and either
> > > >          * userspace can send a valid response or we will clean it up after the
> > > >          * timeout
> > > >          */
> > > > -       switch (response & ~FAN_AUDIT) {
> > > > -       case FAN_ALLOW:
> > > > -       case FAN_DENY:
> > > > -               break;
> > > > -       default:
> > > > -               return -EINVAL;
> > > > -       }
> > > > -
> > > > -       if (fd < 0)
> > > > +       if (FAN_INVALID_RESPONSE_MASK(response))
> > >
> > > That is a logic change, because now the response value of 0 becomes valid.
> > >
> > > Since you did not document this change in the commit message I assume this was
> > > non intentional?
> > > However, this behavior change is something that I did ask for, but it should be
> > > done is a separate commit:
> > >
> > >  /* These are NOT bitwise flags.  Both bits can be used together.  */
> > > #define FAN_TEST          0x00
> > > #define FAN_ALLOW       0x01
> > > #define FAN_DENY        0x02
> > > #define FANOTIFY_RESPONSE_ACCESS \
> > >             (FAN_TEST|FAN_ALLOW | FAN_DENY)
> > >
> > > ...
> > > int access = response & FANOTIFY_RESPONSE_ACCESS;
> > >
> > > 1. Do return EINVAL for access == 0
> > > 2. Let all the rest of the EINVAL checks run (including extra type)
> > > 3. Move if (fd < 0) to last check
> > > 4. Add if (!access) return 0 before if (fd < 0)
> > >
> > > That will provide a mechanism for userspace to probe the
> > > kernel support for extra types in general and specific types
> > > that it may respond with.
> >
> > I have to admit I didn't quite grok your suggestion here although I
> > understand (and agree with) the general direction of the proposal :). Maybe
> > code would explain it better what you have in mind?
> >
>
> +/* These are NOT bitwise flags.  Both bits can be used together.  */

I realize when reading this that this comment is weird, because
0x01 and 0x02 cannot currently be used together.
The comment was copied from above FAN_MARK_INODE where it
has the same weirdness.

The meaning is that (response & FANOTIFY_RESPONSE_ACCESS)
is an enum. I am sure that a less confusing phrasing for this comment
can be found.

> +#define FAN_TEST          0x00
> #define FAN_ALLOW       0x01
> #define FAN_DENY        0x02
> #define FAN_AUDIT       0x10    /* Bit mask to create audit record for result */
> +#define FANOTIFY_RESPONSE_ACCESS \
> +            (FAN_TEST|FAN_ALLOW | FAN_DENY)

Thanks,
Amir.