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

[Jan Kara]

On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> On 2022-12-16 17:43, Jan Kara wrote:
> > On Mon 12-12-22 09:06:10, 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, to audit a 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>
> > 
> > Thanks for the patches. They look very good to me. Just two nits below. I
> > can do the small updates on commit if there would be no other changes. But
> > I'd like to get some review from audit guys for patch 3/3 before I commit
> > this.
> 
> I'd prefer to send a followup patch based on your recommendations rather
> than have you modify it.  It does save some back and forth though...

OK, since there are updates to patch 3 as well, I agree this is a better
way forward.

> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index caa1211bac8c..cf3584351e00 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> > >  	return client_fd;
> > >  }
> > >  
> > > +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> > > +					struct fanotify_response_info_audit_rule *friar)
> > 
> > I prefer to keep lines within 80 columns, unless there is really good
> > reason (like with strings) to have them longer.
> 
> Sure.  In this case, it buys us little since the last line is lined up
> with the arguments openning bracket and it one long struct name unless I
> unalign that argument and back up the indent by one.

Yeah, that's what I'd generally do.

> > BTW, why do you call the info structure 'friar'? I feel some language twist
> > escapes me ;)
> 
> Fanotify_Response_Info_Audit_Rule, it is a pronounceable word, and
> besides they have a long reputation for making good beer.  :-D

Aha, ok :) Thanks for explanation.

> > > +{
> > > +	if (fd == FAN_NOFD)
> > > +		return -ENOENT;
> > 
> > I would not test 'fd' in this function at all. After all it is not part of
> > the response info structure and you do check it in
> > process_access_response() anyway.
> 
> I wrestled with that.  I was even tempted to swallow the following fd
> check too, but the flow would not have made as much sense for the
> non-INFO case.
> 
> My understanding from Amir was that FAN_NOFD was only to be sent in in
> conjuction with FAN_INFO to test if a newer kernel was present.

Yes, that is correct. But we not only want to check that FAN_INFO flag is
understood (as you do in your patch) but also whether a particular response
type is understood (which you don't verify for FAN_NOFD). Currently, there
is only one response type (FAN_RESPONSE_INFO_AUDIT_RULE) but if there are
more in the future we need old kernels to refuse new response types even
for FAN_NOFD case.

> I presumed that if FAN_NOFD was present without FAN_INFO that was an
> invalid input to an old kernel.

Yes, that is correct and I agree the conditions I've suggested below are
wrong in that regard and need a bit of tweaking. Thanks for catching it.

> > > +
> > > +	if (info_len != sizeof(*friar))
> > > +		return -EINVAL;
> > > +
> > > +	if (copy_from_user(friar, info, sizeof(*friar)))
> > > +		return -EFAULT;
> > > +
> > > +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > +		return -EINVAL;
> > > +	if (friar->hdr.pad != 0)
> > > +		return -EINVAL;
> > > +	if (friar->hdr.len != sizeof(*friar))
> > > +		return -EINVAL;
> > > +
> > > +	return info_len;
> > > +}
> > > +
> > 
> > ...
> > 
> > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if (fd < 0)
> > > +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > >  		return -EINVAL;
> > >  
> > > -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > +	if (response & FAN_INFO) {
> > > +		ret = process_access_response_info(fd, info, info_len, &friar);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	} else {
> > > +		ret = 0;
> > > +	}
> > > +
> > > +	if (fd < 0)
> > >  		return -EINVAL;
> > 
> > And here I'd do:
> > 
> > 	if (fd == FAN_NOFD)
> > 		return 0;
> > 	if (fd < 0)
> > 		return -EINVAL;
> > 
> > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > extra info is understood by the kernel so that application writing fanotify
> > responses has a way to check which information it can provide to the
> > kernel.
> 
> The reason for including it in process_access_response_info() is to make
> sure that it is included in the FAN_INFO case to detect this extension.
> If it were included here

I see what you're getting at now. So the condition

 	if (fd == FAN_NOFD)
 		return 0;

needs to be moved into 

	if (response & FAN_INFO)

branch after process_access_response_info(). I still prefer to keep it
outside of the process_access_response_info() function itself as it looks
more logical to me. Does it address your concerns?

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