diff mbox series

[RFC,v2,14/34] new step_into() flag: WALK_NOFOLLOW

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

Commit Message

Al Viro Feb. 23, 2020, 1:16 a.m. UTC 
From: Al Viro <viro@zeniv.linux.org.uk>

Tells step_into() not to follow symlinks, regardless of LOOKUP_FOLLOW.
Allows to switch handle_lookup_down() to of step_into(), getting
all follow_managed() and step_into() calls paired.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/namei.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Linus Torvalds Feb. 23, 2020, 2:14 a.m. UTC | #1 
On Sat, Feb 22, 2020 at 5:20 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         if (likely(!d_is_symlink(path->dentry)) ||
> -          !(flags & WALK_FOLLOW || nd->flags & LOOKUP_FOLLOW)) {
> +          !(flags & WALK_FOLLOW || nd->flags & LOOKUP_FOLLOW) ||
> +          flags & WALK_NOFOLLOW) {

Humor me, and don't mix bitwise ops with logical boolean ops without
parentheses, ok?

And yes, the old code did it too, so it's not a new thing.

But as it gets even more complex, let's just generally strive for doing

   (a & b) || (c & d)

instead of

   a & b || c & d

to make it easier to mentally see the grouping.

               Linus
Al Viro Feb. 23, 2020, 10:07 p.m. UTC | #2 
On Sat, Feb 22, 2020 at 06:14:45PM -0800, Linus Torvalds wrote:
> On Sat, Feb 22, 2020 at 5:20 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> >         if (likely(!d_is_symlink(path->dentry)) ||
> > -          !(flags & WALK_FOLLOW || nd->flags & LOOKUP_FOLLOW)) {
> > +          !(flags & WALK_FOLLOW || nd->flags & LOOKUP_FOLLOW) ||
> > +          flags & WALK_NOFOLLOW) {
> 
> Humor me, and don't mix bitwise ops with logical boolean ops without
> parentheses, ok?
> 
> And yes, the old code did it too, so it's not a new thing.
> 
> But as it gets even more complex, let's just generally strive for doing
> 
>    (a & b) || (c & d)
> 
> instead of
> 
>    a & b || c & d
> 
> to make it easier to mentally see the grouping.

Can do...  FWIW, the only case where the normal "'and' is multiplication,
'or' is addition" doesn't give the right result is
	x | y && z
written instead of
	x | (y && z)
where you would be better off rewriting the expression anyway.

FWIW, one of the things in the local pile is this:

commit cc1b6724b32de1be108cf6a5f28dbb5aa424b42f
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Sun Jan 19 12:44:18 2020 -0500

    namei: invert the meaning of WALK_FOLLOW
    
    old flags & WALK_FOLLOW <=> new !(flags & WALK_TRAILING)
    That's what that flag had really been used for.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/namei.c b/fs/namei.c
index 6eb708014d4b..7d938241157f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1786,7 +1786,7 @@ static const char *pick_link(struct nameidata *nd, struct path *link,
 	return NULL;
 }
 
-enum {WALK_FOLLOW = 1, WALK_MORE = 2, WALK_NOFOLLOW = 4};
+enum {WALK_TRAILING = 1, WALK_MORE = 2, WALK_NOFOLLOW = 4};
 
 /*
  * Do we need to follow links? We _really_ want to be able
@@ -1805,7 +1805,7 @@ static const char *step_into(struct nameidata *nd, int flags,
 	if (!(flags & WALK_MORE) && nd->depth)
 		put_link(nd);
 	if (likely(!d_is_symlink(path.dentry)) ||
-	   !(flags & WALK_FOLLOW || nd->flags & LOOKUP_FOLLOW) ||
+	   (flags & WALK_TRAILING && !(nd->flags & LOOKUP_FOLLOW)) ||
 	   flags & WALK_NOFOLLOW) {
 		/* not a symlink or should not follow */
 		path_to_nameidata(&path, nd);
@@ -2157,10 +2157,10 @@ static int link_path_walk(const char *name, struct nameidata *nd)
 			if (!name)
 				return 0;
 			/* last component of nested symlink */
-			link = walk_component(nd, WALK_FOLLOW);
+			link = walk_component(nd, 0);
 		} else {
 			/* not the last component */
-			link = walk_component(nd, WALK_FOLLOW | WALK_MORE);
+			link = walk_component(nd, WALK_MORE);
 		}
 		if (unlikely(link)) {
 			if (IS_ERR(link))
@@ -2288,7 +2288,7 @@ static inline const char *lookup_last(struct nameidata *nd)
 		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
 
 	nd->flags &= ~LOOKUP_PARENT;
-	link = walk_component(nd, 0);
+	link = walk_component(nd, WALK_TRAILING);
 	if (link) {
 		nd->flags |= LOOKUP_PARENT;
 		nd->stack[0].name = NULL;
@@ -3241,7 +3241,7 @@ static const char *do_last(struct nameidata *nd,
 	}
 
 finish_lookup:
-	link = step_into(nd, 0, dentry, inode, seq);
+	link = step_into(nd, WALK_TRAILING, dentry, inode, seq);
 	if (unlikely(link)) {
 		nd->flags |= LOOKUP_PARENT;
 		nd->flags &= ~(LOOKUP_OPEN|LOOKUP_CREATE|LOOKUP_EXCL);


and I can simply fold adding extra parens into it.  Or I can fold that into
the patch you'd been replying to  - I'm still uncertain about the series
containing WALK_TRAILING...
diff mbox series

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 4c9b633e1981..fe48a8d00ae5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1827,7 +1827,7 @@  static int pick_link(struct nameidata *nd, struct path *link,
 	return 1;
 }
 
-enum {WALK_FOLLOW = 1, WALK_MORE = 2};
+enum {WALK_FOLLOW = 1, WALK_MORE = 2, WALK_NOFOLLOW = 4};
 
 /*
  * Do we need to follow links? We _really_ want to be able
@@ -1841,7 +1841,8 @@  static inline int step_into(struct nameidata *nd, struct path *path,
 	if (!(flags & WALK_MORE) && nd->depth)
 		put_link(nd);
 	if (likely(!d_is_symlink(path->dentry)) ||
-	   !(flags & WALK_FOLLOW || nd->flags & LOOKUP_FOLLOW)) {
+	   !(flags & WALK_FOLLOW || nd->flags & LOOKUP_FOLLOW) ||
+	   flags & WALK_NOFOLLOW) {
 		/* not a symlink or should not follow */
 		path_to_nameidata(path, nd);
 		nd->inode = inode;
@@ -2363,10 +2364,7 @@  static int handle_lookup_down(struct nameidata *nd)
 	err = handle_mounts(nd, nd->path.dentry, &path, &inode, &seq);
 	if (unlikely(err < 0))
 		return err;
-	path_to_nameidata(&path, nd);
-	nd->inode = inode;
-	nd->seq = seq;
-	return 0;
+	return step_into(nd, &path, WALK_NOFOLLOW, inode, seq);
 }
 
 /* Returns 0 and nd will be valid on success; Retuns error, otherwise. */