Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
> 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.