diff mbox

[01/13] VFS: replace {, total_}link_count in task_struct with pointer to nameidata

Message ID 20150316044319.23648.13820.stgit@notabene.brown (mailing list archive)
State New, archived
Headers show

Commit Message

NeilBrown March 16, 2015, 4:43 a.m. UTC
task_struct currently contains two ad-hoc members for use by
the VFS: link_count and total_link_count.
These are only interesting to fs/namei.c, so exposing them
explicitly is poor laying - and has resulted in some questionable
code in staging/lustre.

This patches replaces those with a single pointer to 'struct
nameidata'.
This structure represents the current filename lookup of which
there can only be one per process, and is a natural place to
store link_count and total_link_count.

This will allow the current "nameidata" argument to all
follow_link operations to be removed as current->nameidata
can be used instead.

As there are occasional circumstances where pathname lookup can
recurse, such as through kern_path_locked, we always save and old
current->nameidata (if there is one) when setting a new value, and
make sure any active link_counts are preserved.

Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 drivers/staging/lustre/lustre/llite/symlink.c |   16 ++-------
 fs/namei.c                                    |   47 +++++++++++++++++++------
 include/linux/sched.h                         |    2 +
 3 files changed, 41 insertions(+), 24 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Al Viro March 16, 2015, 7:46 p.m. UTC | #1
On Mon, Mar 16, 2015 at 03:43:19PM +1100, NeilBrown wrote:

> -	if (unlikely(current->total_link_count >= 40))
> +	if (unlikely(current->nameidata->total_link_count >= 40))

Huh?  nd->total_link_count, please.
>  
> -	current->total_link_count++;
> +	current->nameidata->total_link_count++;

Similar.

> @@ -991,8 +1008,8 @@ static int follow_automount(struct path *path, unsigned flags,
>  	    path->dentry->d_inode)
>  		return -EISDIR;
>  
> -	current->total_link_count++;
> -	if (current->total_link_count >= 40)
> +	current->nameidata->total_link_count++;
> +	if (current->nameidata->total_link_count >= 40)
>  		return -ELOOP;

We probably ought to pass nd through follow_mount / follow_automount, instead
of nd->flags, and use nd->total_link_count here.

> -	if (unlikely(current->link_count >= MAX_NESTED_LINKS)) {
> +	if (unlikely(current->nameidata->link_count >= MAX_NESTED_LINKS)) {

Again, nd->link_count.

> @@ -1948,7 +1965,7 @@ static int path_init(int dfd, const char *name, unsigned int flags,
>  	rcu_read_unlock();
>  	return -ECHILD;
>  done:
> -	current->total_link_count = 0;
> +	current->nameidata->total_link_count = 0;

... and again.

>  	return link_path_walk(name, nd);
>  }
>  
> @@ -2027,7 +2044,9 @@ static int path_lookupat(int dfd, const char *name,
>  static int filename_lookup(int dfd, struct filename *name,
>  				unsigned int flags, struct nameidata *nd)
>  {
> -	int retval = path_lookupat(dfd, name->name, flags | LOOKUP_RCU, nd);
> +	int retval;
> +	struct nameidata *saved_nd = set_nameidata(nd);

I'm not sure it's the right place ;-/  I'll play with that a bit and see
if I can get it cleaner...

> -	struct nameidata nd;
> +	struct nameidata nd, *saved = set_nameidata(&nd);
>  	void *cookie;
>  	int res;
>  
> @@ -4441,6 +4465,7 @@ int generic_readlink(struct dentry *dentry, char __user *buffer, int buflen)
>  	res = readlink_copy(buffer, buflen, nd_get_link(&nd));
>  	if (dentry->d_inode->i_op->put_link)
>  		dentry->d_inode->i_op->put_link(dentry, &nd, cookie);
> +	set_nameidata(saved);
>  	return res;

Now, _that_ is broken - get ERR_PTR(...) from ->follow_link() and you've
leaked nameidata.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/staging/lustre/lustre/llite/symlink.c b/drivers/staging/lustre/lustre/llite/symlink.c
index 686b6a574cc5..d7a1c6c48846 100644
--- a/drivers/staging/lustre/lustre/llite/symlink.c
+++ b/drivers/staging/lustre/lustre/llite/symlink.c
@@ -126,18 +126,10 @@  static void *ll_follow_link(struct dentry *dentry, struct nameidata *nd)
 	char *symname = NULL;
 
 	CDEBUG(D_VFSTRACE, "VFS Op\n");
-	/* Limit the recursive symlink depth to 5 instead of default
-	 * 8 links when kernel has 4k stack to prevent stack overflow.
-	 * For 8k stacks we need to limit it to 7 for local servers. */
-	if (THREAD_SIZE < 8192 && current->link_count >= 6) {
-		rc = -ELOOP;
-	} else if (THREAD_SIZE == 8192 && current->link_count >= 8) {
-		rc = -ELOOP;
-	} else {
-		ll_inode_size_lock(inode);
-		rc = ll_readlink_internal(inode, &request, &symname);
-		ll_inode_size_unlock(inode);
-	}
+	ll_inode_size_lock(inode);
+	rc = ll_readlink_internal(inode, &request, &symname);
+	ll_inode_size_unlock(inode);
+
 	if (rc) {
 		ptlrpc_req_finished(request);
 		request = NULL;
diff --git a/fs/namei.c b/fs/namei.c
index c83145af4bfc..184aaafffaa9 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -502,10 +502,27 @@  struct nameidata {
 	unsigned	seq, m_seq;
 	int		last_type;
 	unsigned	depth;
+	int link_count, total_link_count;
 	struct file	*base;
 	char *saved_names[MAX_NESTED_LINKS + 1];
 };
 
+static struct nameidata *set_nameidata(struct nameidata *p)
+{
+	struct nameidata *old = current->nameidata;
+	current->nameidata = p;
+	if (p) {
+		if (!old) {
+			p->link_count = 0;
+			p->total_link_count = 0;
+		} else {
+			p->link_count = old->link_count;
+			p->total_link_count = old->total_link_count;
+		}
+	}
+	return old;
+}
+
 /*
  * Path walking has 2 modes, rcu-walk and ref-walk (see
  * Documentation/filesystems/path-lookup.txt).  In situations when we can't
@@ -863,11 +880,11 @@  follow_link(struct path *link, struct nameidata *nd, void **p)
 		mntget(link->mnt);
 
 	error = -ELOOP;
-	if (unlikely(current->total_link_count >= 40))
+	if (unlikely(current->nameidata->total_link_count >= 40))
 		goto out_put_nd_path;
 
 	cond_resched();
-	current->total_link_count++;
+	current->nameidata->total_link_count++;
 
 	touch_atime(link);
 	nd_set_link(nd, NULL);
@@ -991,8 +1008,8 @@  static int follow_automount(struct path *path, unsigned flags,
 	    path->dentry->d_inode)
 		return -EISDIR;
 
-	current->total_link_count++;
-	if (current->total_link_count >= 40)
+	current->nameidata->total_link_count++;
+	if (current->nameidata->total_link_count >= 40)
 		return -ELOOP;
 
 	mnt = path->dentry->d_op->d_automount(path);
@@ -1621,7 +1638,7 @@  static inline int nested_symlink(struct path *path, struct nameidata *nd)
 {
 	int res;
 
-	if (unlikely(current->link_count >= MAX_NESTED_LINKS)) {
+	if (unlikely(current->nameidata->link_count >= MAX_NESTED_LINKS)) {
 		path_put_conditional(path, nd);
 		path_put(&nd->path);
 		return -ELOOP;
@@ -1629,7 +1646,7 @@  static inline int nested_symlink(struct path *path, struct nameidata *nd)
 	BUG_ON(nd->depth >= MAX_NESTED_LINKS);
 
 	nd->depth++;
-	current->link_count++;
+	current->nameidata->link_count++;
 
 	do {
 		struct path link = *path;
@@ -1642,7 +1659,7 @@  static inline int nested_symlink(struct path *path, struct nameidata *nd)
 		put_link(nd, &link, cookie);
 	} while (res > 0);
 
-	current->link_count--;
+	current->nameidata->link_count--;
 	nd->depth--;
 	return res;
 }
@@ -1948,7 +1965,7 @@  static int path_init(int dfd, const char *name, unsigned int flags,
 	rcu_read_unlock();
 	return -ECHILD;
 done:
-	current->total_link_count = 0;
+	current->nameidata->total_link_count = 0;
 	return link_path_walk(name, nd);
 }
 
@@ -2027,7 +2044,9 @@  static int path_lookupat(int dfd, const char *name,
 static int filename_lookup(int dfd, struct filename *name,
 				unsigned int flags, struct nameidata *nd)
 {
-	int retval = path_lookupat(dfd, name->name, flags | LOOKUP_RCU, nd);
+	int retval;
+	struct nameidata *saved_nd = set_nameidata(nd);
+	retval = path_lookupat(dfd, name->name, flags | LOOKUP_RCU, nd);
 	if (unlikely(retval == -ECHILD))
 		retval = path_lookupat(dfd, name->name, flags, nd);
 	if (unlikely(retval == -ESTALE))
@@ -2036,6 +2055,7 @@  static int filename_lookup(int dfd, struct filename *name,
 
 	if (likely(!retval))
 		audit_inode(name, nd->path.dentry, flags & LOOKUP_PARENT);
+	set_nameidata(saved_nd);
 	return retval;
 }
 
@@ -2343,7 +2363,7 @@  out:
 static int
 path_mountpoint(int dfd, const char *name, struct path *path, unsigned int flags)
 {
-	struct nameidata nd;
+	struct nameidata nd, *saved = set_nameidata(&nd);
 	int err;
 
 	err = path_init(dfd, name, flags, &nd);
@@ -2366,6 +2386,7 @@  path_mountpoint(int dfd, const char *name, struct path *path, unsigned int flags
 	}
 out:
 	path_cleanup(&nd);
+	set_nameidata(saved);
 	return err;
 }
 
@@ -3217,12 +3238,14 @@  static struct file *path_openat(int dfd, struct filename *pathname,
 	struct path path;
 	int opened = 0;
 	int error;
+	struct nameidata *saved_nd;
 
 	file = get_empty_filp();
 	if (IS_ERR(file))
 		return file;
 
 	file->f_flags = op->open_flag;
+	saved_nd = set_nameidata(nd);
 
 	if (unlikely(file->f_flags & __O_TMPFILE)) {
 		error = do_tmpfile(dfd, pathname, nd, flags, op, file, &opened);
@@ -3269,6 +3292,7 @@  out:
 		}
 		file = ERR_PTR(error);
 	}
+	set_nameidata(saved_nd);
 	return file;
 }
 
@@ -4429,7 +4453,7 @@  EXPORT_SYMBOL(readlink_copy);
  */
 int generic_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 {
-	struct nameidata nd;
+	struct nameidata nd, *saved = set_nameidata(&nd);
 	void *cookie;
 	int res;
 
@@ -4441,6 +4465,7 @@  int generic_readlink(struct dentry *dentry, char __user *buffer, int buflen)
 	res = readlink_copy(buffer, buflen, nd_get_link(&nd));
 	if (dentry->d_inode->i_op->put_link)
 		dentry->d_inode->i_op->put_link(dentry, &nd, cookie);
+	set_nameidata(saved);
 	return res;
 }
 EXPORT_SYMBOL(generic_readlink);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6d77432e14ff..b88b9eea169a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1447,7 +1447,7 @@  struct task_struct {
 				       it with task_lock())
 				     - initialized normally by setup_new_exec */
 /* file system info */
-	int link_count, total_link_count;
+	struct nameidata *nameidata;
 #ifdef CONFIG_SYSVIPC
 /* ipc stuff */
 	struct sysv_sem sysvsem;