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

[Jan Kara]

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