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

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

 



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?

>  	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.

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR




[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