Re: [PATCH 2/3] fanotify: define bit map fields to hold response decision context
On Thu, Oct 1, 2020 at 6:24 AM Jan Kara <jack(a)suse.cz> wrote:
On Thu 01-10-20 12:12:19, Jan Kara wrote:
> On Wed 30-09-20 12:12:28, Steve Grubb wrote:
> > This patch defines 2 bit maps within the response variable returned from
Please use "bitmaps" instead of "bit maps".
> So how likely do you find other context types are or that
you'd need to
> further expand the information passed from userspace? Because if there are
> decent changes the expansion will be needed then I'd rather do something
> like:
>
> struct fanotify_response {
> __s32 fd;
> __u16 response;
> __u16 extra_info_type;
> };
>
> which is also backwards compatible and then userspace can set extra_info_type
> and based on the type we'd know how much more bytes we need to copy from
> buf to get additional information for that info type.
>
> This is much more flexible than what you propose and not that complex to
> implement, you get type safety for extra information and don't need to use
> macros to cram everything in u32 etc. Overall cleaner interface I'd say.
Yes, much cleaner. Stealing bits from an integer is one of those
things you do as a last resort when you can't properly change an API.
Considering that APIs always tend to grow and hardly ever shrink, I
would much prefer Jan's suggestion.
Now I realized we need to propagate the extra information through
fanotify
permission event to audit - that can be also done relatively easily. Just
add '__u16 extra_info_type' to fanotify_perm_event and 'char
extra_info_buf[FANOTIFY_MAX_RESPONSE_EXTRA_LEN]' for now for the data. If
we ever grow larger extra info structures, we can always change this to
dynamic allocation but that will be only internal fanotify change,
userspace facing API can stay the same.
That fanotify/audit interface doesn't bother me as much since that is
internal and we can modify that as needed; the userspace/kernel
fanotify API and the audit record are the important things to focus
on.
Simply recording the "extra_info_type" integer and dumping the
"extra_info" as a hex encoded bitstring in the audit record is
probably the most future proof solution, but I'm not sure what Steve's
tooling would want from such a record. It also seems to be in the
spirit of Steve's 3/3 patch.
--
paul moore
www.paul-moore.com