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

[Jan Kara]

Hello Richard!

On Tue 09-08-22 13:22:53, Richard Guy Briggs wrote:
> This patch adds a flag, FAN_INFO and an extensible buffer to provide
> additional information about response decisions.  The buffer contains
> one or more headers defining the information type and the length of the
> following information.  The patch defines one additional information
> type, FAN_RESPONSE_INFO_AUDIT_RULE, an audit rule number.  This will
> allow for the creation of other information types in the future if other
> users of the API identify different needs.
> 
> Suggested-by: Steve Grubb <sgrubb@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Link: https://lore.kernel.org/r/20201001101219.GE17860@xxxxxxxxxxxxxx
> Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> ---
>  fs/notify/fanotify/fanotify.c      |  10 ++-
>  fs/notify/fanotify/fanotify.h      |   2 +
>  fs/notify/fanotify/fanotify_user.c | 104 +++++++++++++++++++++++------
>  include/linux/fanotify.h           |   5 ++
>  include/uapi/linux/fanotify.h      |  27 +++++++-
>  5 files changed, 123 insertions(+), 25 deletions(-)

Amir and Matthew covered most of the comments so let me add just a few I
have on top:

> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index abfa3712c185..14c30e173632 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -428,6 +428,8 @@ struct fanotify_perm_event {
>  	u32 response;			/* userspace answer to the event */
>  	unsigned short state;		/* state of the event */
>  	int fd;		/* fd we passed to userspace for this event */
> +	size_t info_len;
> +	char *info_buf;
>  };

Rather than this, I'd expand struct fanotify_perm_event by adding:

	union info {
		struct fanotify_response_info_header hdr;
		struct fanotify_response_info_audit_rule audit_rule;
	}

at the end of the struct. Currently that is more memory efficient, easier
to code, and more CPU efficient as well. The 'hdr' member of the union can
be used to look at type of the info and then appropriate union member can
be used to get the data. If we ever grow additional info that has
non-trivial size, changing the code to use dynamically allocated buffer as
you do now is very easy. But until that moment it is just overengineering.

> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index ff67ca0d25cc..a4ae953f0e62 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -289,13 +289,18 @@ static int create_fd(struct fsnotify_group *group, struct path *path,
>   */
>  static void finish_permission_event(struct fsnotify_group *group,
>  				    struct fanotify_perm_event *event,
> -				    u32 response)
> +				    struct fanotify_response *response,

Why do you pass struct fanotify_response here instead of plain u32? I don't
see you'd use it anywhere and it introduces some unnecessary churn in other
places.

> +				    size_t info_len, char *info_buf)
>  				    __releases(&group->notification_lock)
>  {
>  	bool destroy = false;
>  
>  	assert_spin_locked(&group->notification_lock);
> -	event->response = response;
> +	event->response = response->response & ~FAN_INFO;

Why do you mask out FAN_INFO here? I don't see a good reason for that.

> +	if (response->response & FAN_INFO) {
> +		event->info_len = info_len;
> +		event->info_buf = info_buf;
> +	}
>  	if (event->state == FAN_EVENT_CANCELED)
>  		destroy = true;
>  	else
...

> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index f1f89132d60e..4d08823a5698 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -180,15 +180,40 @@ struct fanotify_event_info_error {
>  	__u32 error_count;
>  };
>  
> +/*
> + * User space may need to record additional information about its decision.
> + * The extra information type records what kind of information is included.
> + * The default is none. We also define an extra information buffer whose
> + * 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

Why do you define this? I don't see it being used anywhere (in a meaningful
way). You can as well make FAN_RESPONSE_INFO_AUDIT_RULE be type 0...

> +#define FAN_RESPONSE_INFO_AUDIT_RULE	1
> +
>  struct fanotify_response {
>  	__s32 fd;
>  	__u32 response;
>  };
>  
> +struct fanotify_response_info_header {
> +	__u8 type;
> +	__u8 pad;
> +	__u16 len;
> +};
> +
> +struct fanotify_response_info_audit_rule {
> +	struct fanotify_response_info_header hdr;
> +	__u32 audit_rule;
> +};
> +

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR