On Mon, Sep 30, 2024 at 5:42 PM Jan Kara <jack@xxxxxxx> wrote: > > On Fri 27-09-24 14:56:24, Amir Goldstein 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. > > > > In any case, userspace will not know which file failed to > > open, so leave a warning in ksmg 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. > > I have a vague memory that we discussed this before and that you preferred > > to report errno in an extra info field (?), but I have a strong repulsion > > from this altenative, which seems like way over design for the case. > > Hum, I don't remember a proposal for extra info field to hold errno. What I > rather think we talked about was that we would return only the successfully > formatted events, push back the problematic one and on next read from > fanotify group the first event will be the one with error so that will get > returned to userspace. Now this would work but I agree that from userspace > it is kind of difficult to know what went wrong when the read failed (were > the arguments somehow wrong, is this temporary or permanent problem, is it > the fd or something else in the event, etc.) so reporting the error in > place of fd looks like a more convenient option. > > But I wonder: Do we really need to report the error code? We already have > FAN_NOFD with -1 value (which corresponds to EPERM), with pidfd we are > reporting FAN_EPIDFD when its open fails so here we could have FAN_EFD == > -2 in case opening of fd fails for whatever reason? > Well it is hard as it is to understand that went wrong, so the error codes provide some clues for the bug report. ENOENT, ENXIO, EROFS kind of point to the likely reason of failures, so it does not make sense for me to hide this information, which is available. > > 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 leave a warning in ksmg for further investigation. > > + */ > > + if (fd < 0) { > > + pr_warn_ratelimited("fanotify: create_fd(%pd2) failed err=%d\n", > > + path->dentry, fd); > > This is triggerable only by priviledged user so it is not a huge issue but > it still seems wrong that we spam kernel logs with warnings on more or less > normal operation. It is unrealistic that userspace would scrape the logs to > extract these names and furthermove without full path they are not even > telling much. If anything, I'd be willing to accept pr_debug() here which > sysadmin can selectively enable to ease debugging. Even without the full path I could easily understand which file was failing the event in git clone, but sure, pr_debug is a decent compromise. Thanks, Amir.
