On Thu 25-07-24 14:19:46, Josef Bacik wrote:
> From: Amir Goldstein <amir73il@xxxxxxxxx>
>
> With FAN_DENY response, user trying to perform the filesystem operation
> gets an error with errno set to EPERM.
>
> It is useful for hierarchical storage management (HSM) service to be able
> to deny access for reasons more diverse than EPERM, for example EAGAIN,
> if HSM could retry the operation later.
>
> Allow fanotify groups with priority FAN_CLASSS_PRE_CONTENT to responsd
> to permission events with the response value FAN_DENY_ERRNO(errno),
> instead of FAN_DENY to return a custom error.
>
> Limit custom error to values to some errors expected on read(2)/write(2)
^^^ parse error. Perhaps: "Limit custom error values to errors
expected on read..."
> and open(2) of regular files. This list could be extended in the future.
> Userspace can test for legitimate values of FAN_DENY_ERRNO(errno) by
> writing a response to an fanotify group fd with a value of FAN_NOFD
> in the fd field of the response.
>
> The change in fanotify_response is backward compatible, because errno is
> written in the high 8 bits of the 32bit response field and old kernels
> reject respose value with high bits set.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
...
> @@ -258,18 +258,25 @@ static int fanotify_get_response(struct fsnotify_group *group,
> }
>
> /* userspace responded, convert to something usable */
> - switch (event->response & FANOTIFY_RESPONSE_ACCESS) {
> + switch (FAN_RESPONSE_ACCESS(event->response)) {
> case FAN_ALLOW:
> ret = 0;
> break;
> case FAN_DENY:
> + /* Check custom errno from pre-content events */
> + errno = FAN_RESPONSE_ERRNO(event->response);
> + if (errno) {
> + ret = -errno;
> + break;
> + }
> + fallthrough;
> default:
> ret = -EPERM;
> }
>
> /* Check if the response should be audited */
> if (event->response & FAN_AUDIT)
> - audit_fanotify(event->response & ~FAN_AUDIT,
> + audit_fanotify(FAN_RESPONSE_ACCESS(event->response),
> &event->audit_rule);
I think you need to also keep FAN_INFO in the flags not to break some
userspace possibly parsing audit requests.
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index c3c8b2ea80b6..b4d810168521 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -337,11 +337,12 @@ static int process_access_response(struct fsnotify_group *group,
> struct fanotify_perm_event *event;
> int fd = response_struct->fd;
> u32 response = response_struct->response;
> + int errno = FAN_RESPONSE_ERRNO(response);
> int ret = info_len;
> struct fanotify_response_info_audit_rule friar;
>
> - pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%zu\n", __func__,
> - group, fd, response, info, info_len);
> + pr_debug("%s: group=%p fd=%d response=%x errno=%d buf=%p size=%zu\n",
> + __func__, group, fd, response, errno, info, info_len);
> /*
> * make sure the response is valid, if invalid we do nothing and either
> * userspace can send a valid response or we will clean it up after the
> @@ -350,9 +351,33 @@ static int process_access_response(struct fsnotify_group *group,
> if (response & ~FANOTIFY_RESPONSE_VALID_MASK)
> return -EINVAL;
>
> - switch (response & FANOTIFY_RESPONSE_ACCESS) {
> + switch (FAN_RESPONSE_ACCESS(response)) {
> case FAN_ALLOW:
> + if (errno)
> + return -EINVAL;
> + break;
> case FAN_DENY:
> + /* Custom errno is supported only for pre-content groups */
> + if (errno && group->priority != FSNOTIFY_PRIO_PRE_CONTENT)
> + return -EINVAL;
> +
> + /*
> + * Limit errno to values expected on open(2)/read(2)/write(2)
> + * of regular files.
> + */
> + switch (errno) {
> + case 0:
> + case EIO:
> + case EPERM:
> + case EBUSY:
> + case ETXTBSY:
> + case EAGAIN:
> + case ENOSPC:
> + case EDQUOT:
> + break;
> + default:
> + return -EINVAL;
> + }
> break;
> default:
> return -EINVAL;
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index ae6cb2688d52..76d818a7d654 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -132,7 +132,14 @@
> /* These masks check for invalid bits in permission responses. */
> #define FANOTIFY_RESPONSE_ACCESS (FAN_ALLOW | FAN_DENY)
> #define FANOTIFY_RESPONSE_FLAGS (FAN_AUDIT | FAN_INFO)
> -#define FANOTIFY_RESPONSE_VALID_MASK (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS)
> +#define FANOTIFY_RESPONSE_ERRNO (FAN_ERRNO_MASK << FAN_ERRNO_SHIFT)
> +#define FANOTIFY_RESPONSE_VALID_MASK \
> + (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS | \
> + FANOTIFY_RESPONSE_ERRNO)
> +
> +/* errno other than EPERM can specified in upper byte of deny response */
> +#define FAN_RESPONSE_ACCESS(res) ((res) & FANOTIFY_RESPONSE_ACCESS)
> +#define FAN_RESPONSE_ERRNO(res) ((int)((res) >> FAN_ERRNO_SHIFT))
I have to say I find the names FANOTIFY_RESPONSE_ERRNO and
FAN_RESPONSE_ERRNO() (and similarly with FAN_RESPONSE_ACCESS) very similar
and thus confusing. I was staring at it for 5 minutes wondering how comes
it compiles before I realized one prefix is shorter than the other one so
the indentifiers are indeed different. Maybe we'd make these inline
functions instead of macros and name them like:
fanotify_get_response_decision()
fanotify_get_response_errno()
???
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
