Re: [PATCH v4 2/3] fanotify: return only user requested event types in event mask

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Tue, Oct 16, 2018 at 01:52:15PM +0200, Jan Kara wrote:
> 
> Hi,
> 
> the patch looks good. Just couple nits:
> 
> On Thu 11-10-18 21:42:41, Matthew Bobrowski wrote:
> > Modified fanotify_should_send_event() so that it now returns a mask for
>   ^^ Modify

Updated.

> > a event that contains ONLY flags for the event types that have been
>   ^ an

Updated.

> > specifically requested by the user. Flags that may have been included
> > within the event mask, but have not been explicitly requested by the
> > user will not be present in the returned value.
> > 
> > As an example, given the situation where a user requests events of type
> > FAN_OPEN. Traditionally, the event mask returned within an event that
> > occurred on a filesystem object that has been marked for monitoring and is
> > opened, will only ever have the FAN_OPEN bit set. With the introduction of
> > the new flags like FAN_OPEN_EXEC, and perhaps any other future event
> > flags, there is a possibility of the returned event mask containing more
> > than a single bit set, despite having only requested the single event type.
> > Prior to these modifications performed to fanotify_should_send_event(), a
> > user would have received a bundled event mask containing flags FAN_OPEN
> > and FAN_OPEN_EXEC in the instance that a file was opened for execution via
> > execve(), for example. This means that a user would receive event types
> > in the returned event mask that have not been requested. This runs the
> > possibility of breaking existing systems and causing other unforeseen
> > issues.
> > 
> > To mitigate this possibility, fanotify_should_send_event() has been
> > modified to return the event mask containing ONLY event types explicitly
> > requested by the user. This means that we will NOT report events that the
> > user did no set a mask for, and we will NOT report events that the user
> > has set an ignore mask for.
> > 
> > The function name fanotify_should_send_event() has also been updated so
> > that it's more relevant to what it has been designed to do.
> > 
> > Signed-off-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> > ---
> >  fs/notify/fanotify/fanotify.c | 46 ++++++++++++++++++++---------------
> >  1 file changed, 26 insertions(+), 20 deletions(-)
> > 
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index b3e92302ed84..9da334d343b8 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -89,7 +89,13 @@ static int fanotify_get_response(struct fsnotify_group *group,
> >  	return ret;
> >  }
> >  
> > -static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
> > +/*
> > + * This function returns a mask for a event that only contains the flags
> 				      ^^ an

Updated.

> > + * that have been specifically requested by the user. Flags that may have
> > + * been included within the event mask, but have not been ecplicitly
> 								^^ explicitly

Updated.

> > + * requested by the user, will not be present in the returned mask.
> > + */
> > +static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
> >  				       u32 event_mask, const void *data,
> >  				       int data_type)
> 
> <snip>
> 
> >  struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
> > @@ -194,6 +197,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
> >  				 struct fsnotify_iter_info *iter_info)
> >  {
> >  	int ret = 0;
> > +	u32 event_mask = 0;
> >  	struct fanotify_event_info *event;
> >  	struct fsnotify_event *fsn_event;
> >  
> > @@ -211,13 +215,15 @@ static int fanotify_handle_event(struct fsnotify_group *group,
> >  
> >  	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 11);
> >  
> > -	if (!fanotify_should_send_event(iter_info, mask, data, data_type))
> > +	event_mask = fanotify_group_event_mask(iter_info, mask, data,
> > +						data_type);
>         ^^^ Why don't you store the result in 'mask'? You don't need
> the original mask later anyway, it reduces churn and also possibility of
> getting things wrong in the future...

Good point. There's no real reason why we can't overwrite the original
'mask' value with the returned value from fanotify_group_event_mask().
That being said, I've gone ahead and applied updates accordingly.

I will post through the updates within an updated patch series.