On Wed, Oct 2, 2024 at 3:01 PM Jan Kara <jack@xxxxxxx> wrote: > > 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? Makes sense. > 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? > EOVERFLOW? nah, witty but irrelevant. I think EBADF would be a good substitute for FAN_NOFD, but I can live with EINVAL as well. > 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? Yeh that sounds useful. Thanks, Amir.
