lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 28 Jun 2023 13:38:53 +0200
From:   Jan Kara <jack@...e.cz>
To:     Ahelenia Ziemiańska 
        <nabijaczleweli@...ijaczleweli.xyz>
Cc:     Amir Goldstein <amir73il@...il.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jan Kara <jack@...e.cz>,
        Chung-Chiang Cheng <cccheng@...ology.com>, ltp@...ts.linux.it
Subject: Re: [PATCH v4 0/3] fanotify accounting for fs/splice.c

Hello!

On Tue 27-06-23 22:50:46, Ahelenia Ziemiańska wrote:
> Always generate modify out, access in for splice;
> this gets automatically merged with no ugly special cases.
> 
> No changes to 2/3 or 3/3.

Thanks for the patches Ahelena! The code looks fine to me but to be honest
I still have one unresolved question so let me think about it loud here for
documentation purposes :). Do we want fsnotify (any filesystem
notification framework like inotify or fanotify) to actually generate
events on FIFOs? FIFOs are virtual objects and are not part of the
filesystem as such (well, the inode itself and the name is), hence
*filesystem* notification framework does not seem like a great fit to watch
for changes or accesses there. And if we say "yes" for FIFOs, then why not
AF_UNIX sockets? Where do we draw the line? And is it all worth the
trouble?

I understand the convenience of inotify working on FIFOs for the "tail -f"
usecase but then wouldn't this better be fixed in tail(1) itself by using
epoll(7) for FIFOs which, as I've noted in my other reply, does not have
the problem that poll(2) has when there are no writers?

Another issue with FIFOs is that they do not have a concept of file
position. For hierarchical storage usecase we are introducing events that
will report file ranges being modified / accessed and officially supporting
FIFOs is one more special case to deal with.

What is supporting your changes is that fsnotify mostly works for FIFOs
already now (normal reads & writes generate notification) so splice not
working could be viewed as an inconsistency. Sockets (although they are
visible in the filesystem) cannot be open so for them the illusion of being
a file is even weaker.

So overall I guess I'm slightly in favor of making fsnotify generate events
on FIFOs even with splice, provided Amir does not see a big trouble in
supporting this with his upcoming HSM changes.

								Honza

> Ahelenia Ziemiańska (3):
>   splice: always fsnotify_access(in), fsnotify_modify(out) on success
>   splice: fsnotify_access(fd)/fsnotify_modify(fd) in vmsplice
>   splice: fsnotify_access(in), fsnotify_modify(out) on success in tee
> 
>  fs/splice.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> Interdiff against v3:
> diff --git a/fs/splice.c b/fs/splice.c
> index 2ecfccbda956..bdbabc2ebfff 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1184,10 +1184,6 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
>  			out->f_pos = offset;
>  		else
>  			*off_out = offset;
> -
> -		// splice_write-> already marked out
> -		// as modified via vfs_iter_write()
> -		goto noaccessout;
>  	} else if (opipe) {
>  		if (off_out)
>  			return -ESPIPE;
> @@ -1211,11 +1207,10 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out,
>  	} else
>  		return -EINVAL;
>  
> -	if (ret > 0)
> +	if (ret > 0) {
>  		fsnotify_modify(out);
> -noaccessout:
> -	if (ret > 0)
>  		fsnotify_access(in);
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.39.2


-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ