Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1652730821.git.rgb@redhat.com> <1520f08c023d1e919b1a2af161d5a19367b6b4bf.1652730821.git.rgb@redhat.com>
 <CAOQ4uxjV-eNxJ=O_WFTTzspCxXZqpMdh3Fe-N5aB-h1rDr_1hQ@mail.gmail.com> <YoWKPcsySt9cJbtB@madcap2.tricolour.ca>
In-Reply-To: <YoWKPcsySt9cJbtB@madcap2.tricolour.ca>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 19 May 2022 09:03:51 +0300
Message-ID: <CAOQ4uxi+8HUqyGxQBNMqSong92nreOWLKdy9MCrYg8wgW9Dj4g@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] fanotify: define struct members to hold response
 decision context
To:     Richard Guy Briggs <rgb@redhat.com>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Paul Moore <paul@paul-moore.com>,
        Eric Paris <eparis@parisplace.org>,
        Steve Grubb <sgrubb@redhat.com>, Jan Kara <jack@suse.cz>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > 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
>
> Going back to the original code will do that.

Oops, this was supposed to be Do NOT return EINVAL for access == 0
this is the case of FAN_TEST.
The patch I posted later explains that better.

>
> > 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'm still resisting the idea of the TEST flag...  It seems like an
> unneeded extra step and complication...

Please reply to the patch I posted as a reply as point
at said complication. There is no extra step.

>
> The simple presence of the FAN_EXTRA flag should sort it out and could
> even make TEST one of the types.
>

I think you've missed the point of the TEST response code.
The point of the TEST response code is to test whether the
extra type is supported, so TESTS cannot be a type.

You should not think of FAN_TEST as a flag at all, in
fact, it is semantic and can be omitted altogether.

The core of the idea is that:
int access = response & FANOTIFY_RESPONSE_ACCESS;

access is an enum, not a bitwise mask, much like:

unsigned int class = flags & FANOTIFY_CLASS_BITS;
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;

At the moment, userspace must provide a valid access code
either ALLOW or DENY.
Providing no access code (0) is not valid.
I suggest making FAN_EXTRA with no access code a valid
response for testing the EXTRA types support.
(please refer to the patch)

[...]


> > > + * size is determined by the extra information type.
> > > + *
> > > + * If the context type is Rule, then the context following is the rule number
> > > + * that triggered the user space decision.
> > > + */
> > > +
> > > +#define FAN_RESPONSE_INFO_NONE         0
> > > +#define FAN_RESPONSE_INFO_AUDIT_RULE   1
> > > +
> > > +union fanotify_response_extra {
> > > +       __u32 audit_rule;
> > > +};
> > > +
> > >  struct fanotify_response {
> > >         __s32 fd;
> > >         __u32 response;
> > > +       __u32 extra_info_type;
> > > +       union fanotify_response_extra extra_info;
> >
> > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
>
> Again, the intent was to make it fixed for now and change it later if
> needed, but that was a shortsighted approach...
>
> I don't see a need for a len in all response types.  _NONE doesn't need
> any.  _AUDIT_RULE is known to be 32 bits.  Other types can define their
> size and layout as needed, including a len field if it is needed.
>

len is part of a common response info header.
It is meant to make writing generic code.
So Jan's email.

> > I don't know if we want to make this flexible enough to allow for multiple
> > records in the future like we do in events, but the common wisdom of
> > the universe says that if we don't do it, we will need it.
>
> It did occur to me that this could be used for other than audit, hence
> the renaming of the ..."_NONE" macro.
>
> We should be able in the future to define a type that is extensible or
> has multiple records.  We have (2^32) - 2 types left to work with.
>

The way this was done when we first introduced event info
records was the same. We only allowed one type of record
and a single record to begin with, but the format allowed for
extending to multiple records.

struct fanotify_event_metadata already had event_len and
metadata_len, so that was convenient. Supporting multi
records only required that every record has a header with its
own len.

As far as I can tell, the case of fanotify_response is different
because we have the count argument of write(), which serves
as the total response_len.

If we ever want to be able to extend the base fanotify_response,
add fields to it not as extra info records, then we need to add
response_metadata_len to struct fanotify_response, but I think that
would be over design.

Thanks,
Amir.