linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christian Brauner <christian.brauner@ubuntu.com>
To: Jan Kara <jack@suse.cz>
Cc: Amir Goldstein <amir73il@gmail.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Miklos Szeredi <miklos@szeredi.hu>,
	"J. Bruce Fields" <bfields@fieldses.org>
Subject: Re: fsnotify path hooks
Date: Fri, 9 Apr 2021 12:45:46 +0200	[thread overview]
Message-ID: <20210409104546.37i6h2i4ga2xakvp@wittgenstein> (raw)
In-Reply-To: <20210409100811.GA20833@quack2.suse.cz>

On Fri, Apr 09, 2021 at 12:08:11PM +0200, Jan Kara wrote:
> On Thu 08-04-21 18:11:31, Amir Goldstein wrote:
> > > > FYI, I tried your suggested approach above for fsnotify_xattr(),
> > > > but I think I prefer to use an explicit flavor fsnotify_xattr_mnt()
> > > > and a wrapper fsnotify_xattr().
> > > > Pushed WIP to fsnotify_path_hooks branch. It also contains
> > > > some unstashed "fix" patches to tidy up the previous hooks.
> > >
> > > What's in fsnotify_path_hooks branch looks good to me wrt xattr hooks.
> > > I somewhat dislike about e.g. the fsnotify_create() approach you took is
> > > that there are separate hooks fsnotify_create() and fsnotify_create_path()
> > > which expose what is IMO an internal fsnotify detail of what are different
> > > event types. I'd say it is more natural (from VFS POV) to have just a
> > > single hook and fill in as much information as available... Also from
> > 
> > So to be clear, you do NOT want additional wrappers like this and
> > you prefer to have the NULL mnt argument explicit in all callers?
> > 
> > static inline void fsnotify_xattr(struct dentry *dentry)
> > {
> >         fsnotify_xattr_mnt(NULL, dentry);
> > }
> > 
> > For fsnotify_xattr() it does not matter so much, but fsnotify_create/mkdir()
> > have quite a few callers in special filesystems.
> 
> Yes, I prefer explicit NULL mnt argument to make it obvious we are going to

I'm personally not a fan of that passing explicit NULL and one of the
first comments Al made to me about idmapped mounts was sm along the
lines of "don't pass NULL to indicate non-idmapped it's an invitation
for bugs". And I think that's actually a good point.
Maybe we should do something similar to anonymous mount namespaces. For
example, we could introduce an anonymous vfsmount that gets passed by
default. Basically similar to init_user_ns or init_net etc.

> miss something in this case. I agree it's going to be somewhat bigger churn
> but it isn't that bad (10 + 6 callers).
> 
> > > outside view, it is unclear that e.g. vfs_create() will generate some types
> > > of fsnotify events but not all while e.g. do_mknodat() will generate all
> > > fsnotify events. That's why I'm not sure whether a helper like vfs_create()
> > > in your tree is the right abstraction since generating one type of fsnotify
> > > event while not generating another type should be a very conscious decision
> > > of the implementor - basically if you have no other option.
> > 
> > I lost you here.
> 
> Sorry, I was probably too philosophical here ;)
> 
> > Are you ok with vfs_create() vs. vfs_create_nonotify()?
> 
> I'm OK with vfs_create_nonotify(). I have a problem with vfs_create()
> because it generates inode + fs events but does not generate mount events
> which is just strange (although I appreciate the technical reason behind
> it :).
> 
> > How do you propose to change fsnotify hooks in vfs_create()?
> 
> So either pass 'mnt' to vfs_create() - as we discussed, this may be
> actually acceptable these days due to idmapped mounts work - and generate

I would think passing struct vfsmount or even struct path to vfs_*
helpers is acceptable (although I know about the long-standing
resistance) as long as neither are passed down to inode methods
themselves. And that should work since the consensus seems to be to
never generate mnt fanotify events for an underlying mnt where one fs
stacks on top of another (cachefiles, ecryptfs, overlayfs, etc.).
Another argument for passing the vfsmount is that all of those stacking
filesystems already do have access to the relevant struct vfsmount
anyway (As I know from my idmapped mount port of overlayfs for example).

> all events there, or make vfs_create() not generate any fsnotify events and
> create new vfs_create_notify() which will take the 'mnt' and generate
> events. Either is fine with me and more consistent than what you currently
> propose. Thoughts?

One thing, whatever you end up passing to vfs_create() please make sure
to retrieve mnt_userns once so permission checking and object creation
line-up:

int vfs_create(struct vfsmount *mnt, struct inode *dir,
	       struct dentry *dentry, umode_t mode, bool want_excl)
{
	struct user_namespace *mnt_userns;

	mnt_userns = mnt_user_ns(mnt);

	int error = may_create(mnt_userns, dir, dentry);
	if (error)
		return error;

	if (!dir->i_op->create)
		return -EACCES;	/* shouldn't it be ENOSYS? */
	mode &= S_IALLUGO;
	mode |= S_IFREG;
	error = security_inode_create(dir, dentry, mode);
	if (error)
		return error;
	error = dir->i_op->create(mnt_userns, dir, dentry, mode, want_excl);
	if (!error)
		fsnotify_create(mnt, dir, dentry);
	return error;
}

Christian

  reply	other threads:[~2021-04-09 10:45 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-28 15:56 [RFC][PATCH] fanotify: allow setting FAN_CREATE in mount mark mask Amir Goldstein
2021-03-30  7:31 ` Christian Brauner
2021-03-30  9:31   ` Amir Goldstein
2021-03-30 16:24     ` Amir Goldstein
2021-03-31 10:08       ` Christian Brauner
2021-03-31 10:57         ` Amir Goldstein
2021-04-08 11:44         ` open_by_handle_at() in userns Amir Goldstein
2021-04-08 12:55           ` Christian Brauner
2021-04-08 14:15             ` J. Bruce Fields
2021-04-08 15:54               ` Amir Goldstein
2021-04-08 16:08                 ` J. Bruce Fields
2021-04-08 16:48                   ` Frank Filz
2021-04-08 15:34             ` Amir Goldstein
2021-04-08 15:41               ` Christian Brauner
2021-03-30 12:12 ` [RFC][PATCH] fanotify: allow setting FAN_CREATE in mount mark mask Christian Brauner
2021-03-30 12:33   ` Amir Goldstein
2021-03-30 12:53     ` Christian Brauner
2021-03-30 12:55       ` Christian Brauner
2021-03-30 13:54       ` Amir Goldstein
2021-03-30 14:17         ` Christian Brauner
2021-03-30 14:56           ` Amir Goldstein
2021-03-31  9:46             ` Christian Brauner
2021-03-31 11:29               ` Amir Goldstein
2021-03-31 12:17                 ` Christian Brauner
2021-03-31 12:59                   ` Amir Goldstein
2021-03-31 12:54                 ` Jan Kara
2021-03-31 14:06                   ` Amir Goldstein
2021-03-31 20:59                     ` fsnotify path hooks Amir Goldstein
2021-04-01 10:29                       ` Jan Kara
2021-04-01 14:18                         ` Amir Goldstein
2021-04-02  8:20                           ` Amir Goldstein
2021-04-04 10:27                             ` LSM and setxattr helpers Amir Goldstein
2021-04-05 12:23                               ` Christian Brauner
2021-04-05 14:47                               ` Mimi Zohar
2021-04-06 15:43                                 ` Amir Goldstein
2021-04-05 16:18                               ` Casey Schaufler
2021-04-06  8:35                           ` fsnotify path hooks Jan Kara
2021-04-06 18:49                           ` Amir Goldstein
2021-04-08 12:52                             ` Jan Kara
2021-04-08 15:11                               ` Amir Goldstein
2021-04-09 10:08                                 ` Jan Kara
2021-04-09 10:45                                   ` Christian Brauner [this message]
2021-04-20  6:01                                     ` Amir Goldstein
2021-04-20 11:41                                       ` Christian Brauner
2021-04-20 11:58                                         ` Amir Goldstein
2021-04-20 13:38                                         ` Christian Brauner
2021-04-09 13:22                                   ` Amir Goldstein
2021-04-09 14:30                                     ` Al Viro
2021-04-09 14:39                                       ` Christian Brauner
2021-04-09 14:46                                         ` Al Viro
2021-04-09 15:20                                           ` Christian Brauner
2021-04-09 16:06                                       ` Amir Goldstein
2021-04-09 16:09                                         ` Amir Goldstein
2021-04-18 18:51                                   ` Amir Goldstein
2021-04-19  8:08                                     ` Amir Goldstein
2021-04-19 16:41                                 ` Amir Goldstein
2021-04-19 17:02                                   ` Al Viro
2021-04-19 22:04                                     ` Amir Goldstein
2021-04-20  7:53                                       ` Amir Goldstein
2021-03-31 13:06                 ` [RFC][PATCH] fanotify: allow setting FAN_CREATE in mount mark mask J. Bruce Fields
2021-03-30 12:20 ` Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210409104546.37i6h2i4ga2xakvp@wittgenstein \
    --to=christian.brauner@ubuntu.com \
    --cc=amir73il@gmail.com \
    --cc=bfields@fieldses.org \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).