diff mbox series

[RFC,v4,01/69] do_add_mount(): lift lock_mount/unlock_mount into callers

Message ID 20200313235357.2646756-1-viro@ZenIV.linux.org.uk (mailing list archive)
State New, archived
Headers show
Series [RFC,v4,01/69] do_add_mount(): lift lock_mount/unlock_mount into callers | expand

Commit Message

Al Viro March 13, 2020, 11:52 p.m. UTC 
From: Al Viro <viro@zeniv.linux.org.uk>

preparation to finish_automount() fix (next commit)

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/namespace.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

Comments

Al Viro March 15, 2020, 2:23 p.m. UTC | #1 
On Sun, Mar 15, 2020 at 05:41:48PM +0800, Hillf Danton wrote:
> > +	link = walk_component(nd, 0);
> > +	if (link) {
> 
> Need to check that link is not err.

What for?  It will do the right thing anyway.
Al Viro March 15, 2020, 2:24 p.m. UTC | #2 
On Sun, Mar 15, 2020 at 05:53:23PM +0800, Hillf Danton wrote:

> > +	res = step_into(nd, 0, dentry, inode, seq);
> > +	if (unlikely(res)) {
> 
> Need to check that res is not err.
> 
> > +		nd->flags |= LOOKUP_PARENT;
> > +		nd->flags &= ~(LOOKUP_OPEN|LOOKUP_CREATE|LOOKUP_EXCL);
> > +		nd->stack[0].name = NULL;
> > +		return res;
> >  	}

Not really...
Al Viro March 15, 2020, 2:34 p.m. UTC | #3 
On Sun, Mar 15, 2020 at 08:40:07PM +0800, Hillf Danton wrote:
> 
> On Fri, 13 Mar 2020 23:53:26 +0000
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > Don't mess with got_write there - it is guaranteed to be false on
> > entry and it will be set true if and only if we decide to go for
> > truncation and manage to get write access for that.
> > 
> > Don't carry acc_mode through the entire thing - it's only used
> > in that part.  And don't bother with gotos in there - compiler is
> > quite capable of optimizing that.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >  fs/namei.c | 28 +++++++++++-----------------
> >  1 file changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index c85fdfd6b33d..8cdf8ef41194 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3122,9 +3122,9 @@ static const char *do_last(struct nameidata *nd,
> >  	kuid_t dir_uid = nd->inode->i_uid;
> >  	umode_t dir_mode = nd->inode->i_mode;
> >  	int open_flag = op->open_flag;
> > -	bool will_truncate = (open_flag & O_TRUNC) != 0;
> > +	bool do_truncate;
> >  	bool got_write = false;
> 
> Without got_write cut,
> 
> > -	int acc_mode = op->acc_mode;
> > +	int acc_mode;
> >  	unsigned seq;
> >  	struct inode *inode;
> >  	struct dentry *dentry;
> > @@ -3243,36 +3243,30 @@ static const char *do_last(struct nameidata *nd,
> >  		return ERR_PTR(-ENOTDIR);
> >  
> >  finish_open_created:
> > +	do_truncate = false;
> > +	acc_mode = op->acc_mode;
> >  	if (file->f_mode & FMODE_CREATED) {
> >  		/* Don't check for write permission, don't truncate */
> >  		open_flag &= ~O_TRUNC;
> > -		will_truncate = false;
> >  		acc_mode = 0;
> > -	} else if (!d_is_reg(nd->path.dentry)) {
> > -		will_truncate = false;
> > -	}
> > -	if (will_truncate) {
> > +	} else if (d_is_reg(nd->path.dentry) && open_flag & O_TRUNC) {
> >  		error = mnt_want_write(nd->path.mnt);
> >  		if (error)
> >  			return ERR_PTR(error);
> > -		got_write = true;
> > +		do_truncate = true;
> 
> replacing its role with do_truncate probably makes it easier to maintain
> do_last() leaving readers baffling and digging in logs after going up
> and down the code but failing to grip what is inteded in this patch.
> 
> And that kink can be fixed perhaps simply by not axing this got_write.

I'm sorry, I'd been unable to parse that ;-/  What do you mean?
Al Viro March 15, 2020, 2:37 p.m. UTC | #4 
On Sun, Mar 15, 2020 at 06:29:05PM +0800, Hillf Danton wrote:

> > And make link_path_walk() _always_ assign ->last_type - in the only
> > case when the value at the entry might survive to the return that value
> > is always LAST_ROOT, inherited from path_init().  Move that assignment
> > from path_init() into the beginning of link_path_walk(), to consolidate
> > the things.
> > 
> If you agree that the move is not a huge change, drop it for the sake of
> init in order to close the time window of a random last_type.

What time window of a random last_type and what does "drop for the sake of
init" mean?  Confused...
diff mbox series

Patch

diff --git a/fs/namespace.c b/fs/namespace.c
index 85b5f7bea82e..dcd015fafe01 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2697,45 +2697,32 @@  static int do_move_mount_old(struct path *path, const char *old_name)
 /*
  * add a mount into a namespace's mount tree
  */
-static int do_add_mount(struct mount *newmnt, struct path *path, int mnt_flags)
+static int do_add_mount(struct mount *newmnt, struct mountpoint *mp,
+			struct path *path, int mnt_flags)
 {
-	struct mountpoint *mp;
-	struct mount *parent;
-	int err;
+	struct mount *parent = real_mount(path->mnt);
 
 	mnt_flags &= ~MNT_INTERNAL_FLAGS;
 
-	mp = lock_mount(path);
-	if (IS_ERR(mp))
-		return PTR_ERR(mp);
-
-	parent = real_mount(path->mnt);
-	err = -EINVAL;
 	if (unlikely(!check_mnt(parent))) {
 		/* that's acceptable only for automounts done in private ns */
 		if (!(mnt_flags & MNT_SHRINKABLE))
-			goto unlock;
+			return -EINVAL;
 		/* ... and for those we'd better have mountpoint still alive */
 		if (!parent->mnt_ns)
-			goto unlock;
+			return -EINVAL;
 	}
 
 	/* Refuse the same filesystem on the same mount point */
-	err = -EBUSY;
 	if (path->mnt->mnt_sb == newmnt->mnt.mnt_sb &&
 	    path->mnt->mnt_root == path->dentry)
-		goto unlock;
+		return -EBUSY;
 
-	err = -EINVAL;
 	if (d_is_symlink(newmnt->mnt.mnt_root))
-		goto unlock;
+		return -EINVAL;
 
 	newmnt->mnt.mnt_flags = mnt_flags;
-	err = graft_tree(newmnt, parent, mp);
-
-unlock:
-	unlock_mount(mp);
-	return err;
+	return graft_tree(newmnt, parent, mp);
 }
 
 static bool mount_too_revealing(const struct super_block *sb, int *new_mnt_flags);
@@ -2748,6 +2735,7 @@  static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
 			   unsigned int mnt_flags)
 {
 	struct vfsmount *mnt;
+	struct mountpoint *mp;
 	struct super_block *sb = fc->root->d_sb;
 	int error;
 
@@ -2768,7 +2756,13 @@  static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
 
 	mnt_warn_timestamp_expiry(mountpoint, mnt);
 
-	error = do_add_mount(real_mount(mnt), mountpoint, mnt_flags);
+	mp = lock_mount(mountpoint);
+	if (IS_ERR(mp)) {
+		mntput(mnt);
+		return PTR_ERR(mp);
+	}
+	error = do_add_mount(real_mount(mnt), mp, mountpoint, mnt_flags);
+	unlock_mount(mp);
 	if (error < 0)
 		mntput(mnt);
 	return error;
@@ -2830,6 +2824,7 @@  static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
 int finish_automount(struct vfsmount *m, struct path *path)
 {
 	struct mount *mnt = real_mount(m);
+	struct mountpoint *mp;
 	int err;
 	/* The new mount record should have at least 2 refs to prevent it being
 	 * expired before we get a chance to add it
@@ -2842,7 +2837,13 @@  int finish_automount(struct vfsmount *m, struct path *path)
 		goto fail;
 	}
 
-	err = do_add_mount(mnt, path, path->mnt->mnt_flags | MNT_SHRINKABLE);
+	mp = lock_mount(path);
+	if (IS_ERR(mp)) {
+		err = PTR_ERR(mp);
+		goto fail;
+	}
+	err = do_add_mount(mnt, mp, path, path->mnt->mnt_flags | MNT_SHRINKABLE);
+	unlock_mount(mp);
 	if (!err)
 		return 0;
 fail: