Re: [PATCH v2] fanotify: allow reporting errors on failure to open fd

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 3, 2024 at 4:29 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> When working in "fd mode", fanotify_read() needs to open an fd
> from a dentry to report event->fd to userspace.
>
> Opening an fd from dentry can fail for several reasons.
> For example, when tasks are gone and we try to open their
> /proc files or we try to open a WRONLY file like in sysfs
> or when trying to open a file that was deleted on the
> remote network server.
>
> Add a new flag FAN_REPORT_FD_ERROR for fanotify_init().
> For a group with FAN_REPORT_FD_ERROR, we will send the
> event with the error instead of the open fd, otherwise
> userspace may not get the error at all.
>
> The FAN_REPORT_FD_ERROR flag is not allowed for groups in "fid mode"
> which do not use open fd's as the object identifier.
>
> For ean overflow event, we report -EBADF to avoid confusing FAN_NOFD
> with -EPERM.  Similarly for pidfd open errors we report either -ESRCH
> or the open error instead of FAN_NOPIDFD and FAN_EPIDFD.
>
> In any case, userspace will not know which file failed to
> open, so add a debug print for further investigation.
>
> Reported-by: Krishna Vivek Vitta <kvitta@xxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-fsdevel/SI2P153MB07182F3424619EDDD1F393EED46D2@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> Jan,
>
> This is my proposal for a slightly better UAPI for error reporting,
> taking into account your review comments on v1 [1].
>
> I have written some basic LTP tests for the simple cases [2], but
> not yet for actual open errors.

FWIW, pushed a new test case for reporting -EROFS error either to
fanotify_read() or in event->fd.

Thanks,
Amir.

>
> I tested the open error manually using an enhanced fanotify_example [3]
> and the reproducer of the 9p open unlinked file issue [4]:
>
> $ ./fanotify_example /vtmp/
> Press enter key to terminate.
> Listening for events.
> FAN_OPEN_PERM: File /vtmp/config.lock
> FAN_CLOSE_WRITE: fd open failed: No such file or directory
>
> And the debug print in kmsg:
> [ 1836.619957] fanotify: create_fd(/config.lock) failed err=-2
>
> fanotify_read() can still return an error with FAN_REPORT_FD_ERROR,
> but not for a failure to open an fd.
>
> Thanks,
> Amir.
>
> Changes since v1:
> - Change pr_warn() => pr_debug()
> - Restrict FAN_REPORT_FD_ERROR to group in fd mode
> - Report fd error also for pidfd errors
> - Report -EBAFD instead of FAN_NOFD in overflow event
>
> [1] https://lore.kernel.org/linux-fsdevel/20240927125624.2198202-1-amir73il@xxxxxxxxx/
> [2] https://github.com/amir73il/ltp/commits/fan_fd_error
> [3] https://github.com/amir73il/fsnotify-utils/blob/fan_report_fd_error/src/test/fanotify_example.c
> [4] https://lore.kernel.org/linux-fsdevel/CAOQ4uxgRnzB0E2ESeqgZBHW++zyRj8-VmvB38Vxm5OXgr=EM9g@xxxxxxxxxxxxxx/
>
>  fs/notify/fanotify/fanotify_user.c | 83 +++++++++++++++++++++---------
>  include/linux/fanotify.h           |  1 +
>  include/uapi/linux/fanotify.h      |  1 +
>  3 files changed, 62 insertions(+), 23 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 9644bc72e457..37a0dd8ae883 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -266,13 +266,6 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
>                                group->fanotify_data.f_flags | __FMODE_NONOTIFY,
>                                current_cred());
>         if (IS_ERR(new_file)) {
> -               /*
> -                * we still send an event even if we can't open the file.  this
> -                * can happen when say tasks are gone and we try to open their
> -                * /proc files or we try to open a WRONLY file like in sysfs
> -                * we just send the errno to userspace since there isn't much
> -                * else we can do.
> -                */
>                 put_unused_fd(client_fd);
>                 client_fd = PTR_ERR(new_file);
>         } else {
> @@ -653,6 +646,19 @@ static int copy_info_records_to_user(struct fanotify_event *event,
>         return total_bytes;
>  }
>
> +/* Determine with value to report in event->fd */
> +static int event_fd_error(struct fsnotify_group *group, int fd, int nofd)
> +{
> +       /* An unprivileged user should never get an open fd or specific error */
> +       if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV))
> +               return nofd;
> +
> +       if (fd >= 0 || FAN_GROUP_FLAG(group, FAN_REPORT_FD_ERROR))
> +               return fd;
> +
> +       return nofd;
> +}
> +
>  static ssize_t copy_event_to_user(struct fsnotify_group *group,
>                                   struct fanotify_event *event,
>                                   char __user *buf, size_t count)
> @@ -691,8 +697,32 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>         if (!FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
>             path && path->mnt && path->dentry) {
>                 fd = create_fd(group, path, &f);
> -               if (fd < 0)
> -                       return fd;
> +               /*
> +                * Opening an fd from dentry can fail for several reasons.
> +                * For example, when tasks are gone and we try to open their
> +                * /proc files or we try to open a WRONLY file like in sysfs
> +                * or when trying to open a file that was deleted on the
> +                * remote network server.
> +                *
> +                * For a group with FAN_REPORT_FD_ERROR, we will send the
> +                * event with the error instead of the open fd, otherwise
> +                * Userspace may not get the error at all.
> +                * In any case, userspace will not know which file failed to
> +                * open, so add a debug print for further investigation.
> +                */
> +               if (fd < 0) {
> +                       pr_debug("fanotify: create_fd(%pd2) failed err=%d\n",
> +                                path->dentry, fd);
> +                       if (!FAN_GROUP_FLAG(group, FAN_REPORT_FD_ERROR))
> +                               return fd;
> +               }
> +       } else {
> +               /*
> +                * For a group with FAN_REPORT_FD_ERROR, report an event with
> +                * no file, such as an overflow event with -BADF instead of
> +                * FAN_NOFD, because FAN_NOFD collides with -EPERM.
> +                */
> +               fd = event_fd_error(group, -EBADF, FAN_NOFD);
>         }
>         metadata.fd = fd;
>
> @@ -709,17 +739,17 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>                  * The PIDTYPE_TGID check for an event->pid is performed
>                  * preemptively in an attempt to catch out cases where the event
>                  * listener reads events after the event generating process has
> -                * already terminated. Report FAN_NOPIDFD to the event listener
> -                * in those cases, with all other pidfd creation errors being
> -                * reported as FAN_EPIDFD.
> +                * already terminated.  Depending on flag FAN_REPORT_FD_ERROR,
> +                * report either -ESRCH or FAN_NOPIDFD to the event listener in
> +                * those cases with all other pidfd creation errors reported as
> +                * the error code itself or as FAN_EPIDFD.
>                  */
>                 if (metadata.pid == 0 ||
>                     !pid_has_task(event->pid, PIDTYPE_TGID)) {
> -                       pidfd = FAN_NOPIDFD;
> +                       pidfd = event_fd_error(group, -ESRCH, FAN_NOPIDFD);
>                 } else {
>                         pidfd = pidfd_prepare(event->pid, 0, &pidfd_file);
> -                       if (pidfd < 0)
> -                               pidfd = FAN_EPIDFD;
> +                       pidfd = event_fd_error(group, pidfd, FAN_NOPIDFD);
>                 }
>         }
>
> @@ -737,9 +767,6 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>         buf += FAN_EVENT_METADATA_LEN;
>         count -= FAN_EVENT_METADATA_LEN;
>
> -       if (fanotify_is_perm_event(event->mask))
> -               FANOTIFY_PERM(event)->fd = fd;
> -
>         if (info_mode) {
>                 ret = copy_info_records_to_user(event, info, info_mode, pidfd,
>                                                 buf, count);
> @@ -753,15 +780,18 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
>         if (pidfd_file)
>                 fd_install(pidfd, pidfd_file);
>
> +       if (fanotify_is_perm_event(event->mask))
> +               FANOTIFY_PERM(event)->fd = fd;
> +
>         return metadata.event_len;
>
>  out_close_fd:
> -       if (fd != FAN_NOFD) {
> +       if (f) {
>                 put_unused_fd(fd);
>                 fput(f);
>         }
>
> -       if (pidfd >= 0) {
> +       if (pidfd_file) {
>                 put_unused_fd(pidfd);
>                 fput(pidfd_file);
>         }
> @@ -845,7 +875,7 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
>                 if (!fanotify_is_perm_event(event->mask)) {
>                         fsnotify_destroy_event(group, &event->fse);
>                 } else {
> -                       if (ret <= 0) {
> +                       if (ret <= 0 || FANOTIFY_PERM(event)->fd < 0) {
>                                 spin_lock(&group->notification_lock);
>                                 finish_permission_event(group,
>                                         FANOTIFY_PERM(event), FAN_DENY, NULL);
> @@ -1453,7 +1483,14 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>                 return -EINVAL;
>         }
>
> -       if (fid_mode && class != FAN_CLASS_NOTIF)
> +       /*
> +        * Legacy fanotify mode reports open fd's in event->fd.
> +        * With fid mode, open fd's are not reported and event->fd is FAN_NOFD.
> +        * High priority classes require reporting open fd's.
> +        * FAN_REPORT_FD_ERROR is only allowed when reporting open fd's.
> +        */
> +       if (fid_mode &&
> +           (class != FAN_CLASS_NOTIF || flags & FAN_REPORT_FD_ERROR))
>                 return -EINVAL;
>
>         /*
> @@ -1954,7 +1991,7 @@ static int __init fanotify_user_setup(void)
>                                      FANOTIFY_DEFAULT_MAX_USER_MARKS);
>
>         BUILD_BUG_ON(FANOTIFY_INIT_FLAGS & FANOTIFY_INTERNAL_GROUP_FLAGS);
> -       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 12);
> +       BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 13);
>         BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 11);
>
>         fanotify_mark_cache = KMEM_CACHE(fanotify_mark,
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index 4f1c4f603118..89ff45bd6f01 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -36,6 +36,7 @@
>  #define FANOTIFY_ADMIN_INIT_FLAGS      (FANOTIFY_PERM_CLASSES | \
>                                          FAN_REPORT_TID | \
>                                          FAN_REPORT_PIDFD | \
> +                                        FAN_REPORT_FD_ERROR | \
>                                          FAN_UNLIMITED_QUEUE | \
>                                          FAN_UNLIMITED_MARKS)
>
> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index a37de58ca571..34f221d3a1b9 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -60,6 +60,7 @@
>  #define FAN_REPORT_DIR_FID     0x00000400      /* Report unique directory id */
>  #define FAN_REPORT_NAME                0x00000800      /* Report events with name */
>  #define FAN_REPORT_TARGET_FID  0x00001000      /* Report dirent target id  */
> +#define FAN_REPORT_FD_ERROR    0x00002000      /* event->fd can report error */
>
>  /* Convenience macro - FAN_REPORT_NAME requires FAN_REPORT_DIR_FID */
>  #define FAN_REPORT_DFID_NAME   (FAN_REPORT_DIR_FID | FAN_REPORT_NAME)
> --
> 2.34.1
>





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux