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

On Mon 16-01-23 15:42:29, Richard Guy Briggs wrote:

On 2023-01-03 13:42, Jan Kara wrote:

quoted

On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:

quoted

quoted

quoted

+
+	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;
+}
+

...

quoted

@@ -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?

Ok.  Note that this does not return zero to userspace, since this
function's return value is added to the size of the struct
fanotify_response when there is no error.

Right, good point. 0 is not a good return value in this case.

For that reason, I think it makes more sense to return -ENOENT, or some
other unused error code that fits, unless you think it is acceptable to
return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
this.

Yeah, my intention was to indicate "success" to userspace so I'd like to
return whatever we return for the case when struct fanotify_response is
accepted for a normal file descriptor - looks like info_len is the right
value. Thanks!

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR