On Mon 30-09-24 18:14:33, Amir Goldstein wrote: > 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. OK, fair enough. I was kind of hoping we could avoid the feature flag but probably we cannot even if we added just FAN_EFD. But I still have a bit of problem with FAN_NOFD overlapping with -EPERM. I guess it kind of makes sense to return -EPERM in that field for unpriviledged groups but we return FAN_NOFD also for events without path attached and there it gets somewhat confusing... Perhaps we should refuse FAN_REPORT_FD_ERROR for groups in fid mode? That would still leave overflow events so instead of setting fd to FAN_NOFD, we could set it to -EINVAL to preserve the property that fd is either -errno or fd number? And then I have a second question about pidfd. Should FAN_REPORT_FD_ERROR influence it in the same way? Something like -ESRCH if the process already exited and otherwise pass back the errno? Honza > > > 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. -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
