Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
On Thu 19-05-22 09:03:51, Amir Goldstein wrote:
> > > + * 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.
Yes. The reason why I want 'type' + 'len' information for every extra
response type is so that the code can be layered properly. Fanotify has no
bussiness in understanding the details of the additional info (or its
expected length) passed from userspace. That is the knowledge that should
stay within the subsystem this info is for. So the length of info record
needs to be passed in the generic info header.
To give an example imagine a situation when we'd like to attach two
different info records to a response, each for a different subsystem. Then
fanotify has to split response buffer and pass each info to the target
subsystem or maybe we'd just pass all info to both subsystems and define
they should ignore info they don't understand but in either case we need to
have a way to be able to separate different info records without apriori
knowledge what they actually mean or what is their expected length.
> > 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.
Yes.
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.
Yeah, I don't think that will happen. The standard response metadata is
basically fixed by backward compatibility constraints. If we need to extend
it in the future, I would prefer the extension to be in a form of an extra
info record.
Honza
--
Jan Kara <jack(a)suse.com>
SUSE Labs, CR