Re: [PATCH 05/20] afs: Implement @sys substitution handling

From: David Howells
Date: Fri Apr 06 2018 - 06:32:21 EST

Next message: Gaurav Dhingra: "Re: [PATCH v2] staging: greybus: Fix warning to limit chars per line"
Previous message: Jarkko Sakkinen: "Re: [PATCH v4 0/4] Migrate all TPM 2.0 commands to use struct tpm_buf"
In reply to: David Howells: "Re: [PATCH 05/20] afs: Implement @sys substitution handling"
Next in thread: David Howells: "[PATCH 06/20] afs: Implement @cell substitution handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Al,

Here's an updated set of changes.

I've improved the sysnames list handling and made it install a list consisting
of a blank name if the names are cleared (I'm told this is what should
happen).

I've also reinstated the "." and ".." checks in afs_lookup_rec() to catch
substitution creating them by substituting a blank name.

David
---
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index a14ea4280590..45a472f3d368 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -760,6 +760,60 @@ static struct inode *afs_do_lookup(struct inode *dir, struct dentry *dentry,
return inode;
}

+/*
+ * Do a parallel recursive lookup.
+ *
+ * Ideally, we'd call lookup_one_len(), but we can't because we'd need to be
+ * holding i_mutex but we only hold i_rwsem for read.
+ */
+struct dentry *afs_lookup_rec(struct dentry *dir, const char *name, int len)
+{
+ DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
+ struct dentry *dentry, *old;
+ struct inode *inode = dir->d_inode;
+ struct qstr this;
+ int ret;
+
+ if (len <= 0)
+ return ERR_PTR(-EINVAL);
+ if (name[0] == '.' &&
+ (len < 2 || (len == 2 && name[1] == '.')))
+ return ERR_PTR(-EINVAL);
+ if (unlikely(IS_DEADDIR(inode)))
+ return ERR_PTR(-ESTALE);
+
+ this.name = name;
+ this.len = len;
+ this.hash = full_name_hash(dir, name, len);
+
+again:
+ dentry = d_alloc_parallel(dir, &this, &wq);
+ if (IS_ERR(dentry))
+ return ERR_CAST(dentry);
+
+ if (unlikely(!d_in_lookup(dentry))) {
+ ret = dentry->d_op->d_revalidate(dentry, 0);
+ if (unlikely(ret <= 0)) {
+ if (!ret) {
+ d_invalidate(dentry);
+ dput(dentry);
+ goto again;
+ }
+ dput(dentry);
+ dentry = ERR_PTR(ret);
+ }
+ } else {
+ old = inode->i_op->lookup(inode, dentry, 0);
+ d_lookup_done(dentry); /* Clean up wq */
+ if (unlikely(old)) {
+ dput(dentry);
+ dentry = old;
+ }
+ }
+
+ return dentry;
+}
+
/*
* Look up an entry in a directory with @sys substitution.
*/
@@ -785,43 +839,33 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry,
p += dentry->d_name.len - 4;
}

- /* There is an ordered list of substitutes that we have to try */
- for (i = 0; i < AFS_NR_SYSNAME; i++) {
- read_lock(&net->sysnames_lock);
- subs = net->sysnames;
- if (!subs || i >= subs->nr) {
- read_unlock(&net->sysnames_lock);
- goto noent;
- }
+ /* There is an ordered list of substitutes that we have to try. */
+ read_lock(&net->sysnames_lock);
+ subs = net->sysnames;
+ refcount_inc(&subs->usage);
+ read_unlock(&net->sysnames_lock);

+ for (i = 0; i < subs->nr; i++) {
name = subs->subs[i];
- if (!name) {
- read_unlock(&net->sysnames_lock);
- goto noent;
- }
-
len = dentry->d_name.len - 4 + strlen(name);
if (len >= AFSNAMEMAX) {
- read_unlock(&net->sysnames_lock);
ret = ERR_PTR(-ENAMETOOLONG);
- goto out_b;
+ goto out_s;
}

strcpy(p, name);
- read_unlock(&net->sysnames_lock);
-
- ret = lookup_one_len(buf, parent, len);
+ ret = afs_lookup_rec(parent, buf, len);
if (IS_ERR(ret) || d_is_positive(ret))
- goto out_b;
+ goto out_s;
dput(ret);
}

-noent:
/* We don't want to d_add() the @sys dentry here as we don't want to
* the cached dentry to hide changes to the sysnames list.
*/
ret = NULL;
-out_b:
+out_s:
+ afs_put_sysnames(subs);
kfree(buf);
out_p:
dput(parent);
diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index b70380e5d0e3..42d03a80310d 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -125,7 +125,7 @@ static struct dentry *afs_lookup_atcell(struct dentry *dentry)
if (!cell)
goto out_n;

- ret = lookup_one_len(name, parent, len);
+ ret = afs_lookup_rec(parent, name, len);
if (IS_ERR(ret) || d_is_positive(ret))
goto out_n;

diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 70dd41e06363..6e98acc1b43f 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -207,9 +207,11 @@ extern struct file_system_type afs_fs_type;
*/
struct afs_sysnames {
#define AFS_NR_SYSNAME 16
- char *subs[AFS_NR_SYSNAME];
- unsigned short nr;
- short error;
+ char *subs[AFS_NR_SYSNAME];
+ refcount_t usage;
+ unsigned short nr;
+ short error;
+ char blank[1];
};

/*
@@ -682,6 +684,7 @@ extern const struct inode_operations afs_dir_inode_operations;
extern const struct address_space_operations afs_dir_aops;
extern const struct dentry_operations afs_fs_dentry_operations;

+extern struct dentry *afs_lookup_rec(struct dentry *, const char *, int);
extern void afs_d_release(struct dentry *);

/*
@@ -849,6 +852,7 @@ extern int __net_init afs_proc_init(struct afs_net *);
extern void __net_exit afs_proc_cleanup(struct afs_net *);
extern int afs_proc_cell_setup(struct afs_net *, struct afs_cell *);
extern void afs_proc_cell_remove(struct afs_net *, struct afs_cell *);
+extern void afs_put_sysnames(struct afs_sysnames *);

/*
* rotate.c
diff --git a/fs/afs/main.c b/fs/afs/main.c
index 01a55d25d064..d7560168b3bf 100644
--- a/fs/afs/main.c
+++ b/fs/afs/main.c
@@ -104,6 +104,7 @@ static int __net_init afs_net_init(struct afs_net *net)
goto error_sysnames;
sysnames->subs[0] = (char *)&afs_init_sysname;
sysnames->nr = 1;
+ refcount_set(&sysnames->usage, 1);
net->sysnames = sysnames;
rwlock_init(&net->sysnames_lock);

@@ -132,7 +133,7 @@ static int __net_init afs_net_init(struct afs_net *net)
net->live = false;
afs_proc_cleanup(net);
error_proc:
- kfree(net->sysnames);
+ afs_put_sysnames(net->sysnames);
error_sysnames:
net->live = false;
return ret;
@@ -148,6 +149,7 @@ static void __net_exit afs_net_exit(struct afs_net *net)
afs_purge_servers(net);
afs_close_socket(net);
afs_proc_cleanup(net);
+ afs_put_sysnames(net->sysnames);
}

/*
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 870b0bad03d0..839a22280606 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -393,6 +393,12 @@ static ssize_t afs_proc_rootcell_write(struct file *file,
if (IS_ERR(kbuf))
return PTR_ERR(kbuf);

+ ret = -EINVAL;
+ if (kbuf[0] == '.')
+ goto out;
+ if (memchr(kbuf, '/', size))
+ goto out;
+
/* trim to first NL */
s = memchr(kbuf, '\n', size);
if (s)
@@ -405,6 +411,7 @@ static ssize_t afs_proc_rootcell_write(struct file *file,
if (ret >= 0)
ret = size; /* consume everything, always */

+out:
kfree(kbuf);
_leave(" = %d", ret);
return ret;
@@ -699,13 +706,14 @@ static int afs_proc_servers_show(struct seq_file *m, void *v)
return 0;
}

-static void afs_sysnames_free(struct afs_sysnames *sysnames)
+void afs_put_sysnames(struct afs_sysnames *sysnames)
{
int i;

- if (sysnames) {
+ if (sysnames && refcount_dec_and_test(&sysnames->usage)) {
for (i = 0; i < sysnames->nr; i++)
- if (sysnames->subs[i] != afs_init_sysname)
+ if (sysnames->subs[i] != afs_init_sysname &&
+ sysnames->subs[i] != sysnames->blank)
kfree(sysnames->subs[i]);
}
}
@@ -732,6 +740,7 @@ static int afs_proc_sysname_open(struct inode *inode, struct file *file)
return -ENOMEM;
}

+ refcount_set(&sysnames->usage, 1);
m = file->private_data;
m->private = sysnames;
}
@@ -775,27 +784,28 @@ static ssize_t afs_proc_sysname_write(struct file *file,
len = strlen(s);
if (len == 0)
continue;
- if (len >= AFSNAMEMAX) {
- sysnames->error = -ENAMETOOLONG;
- ret = -ENAMETOOLONG;
- goto out;
- }
+ ret = -ENAMETOOLONG;
+ if (len >= AFSNAMEMAX)
+ goto error;
+
if (len >= 4 &&
s[len - 4] == '@' &&
s[len - 3] == 's' &&
s[len - 2] == 'y' &&
- s[len - 1] == 's') {
+ s[len - 1] == 's')
/* Protect against recursion */
- sysnames->error = -EINVAL;
- ret = -EINVAL;
- goto out;
- }
+ goto invalid;
+
+ if (s[0] == '.' &&
+ (len < 2 || (len == 2 && s[1] == '.')))
+ goto invalid;
+
+ if (memchr(s, '/', len))
+ goto invalid;

- if (sysnames->nr >= AFS_NR_SYSNAME) {
- sysnames->error = -EFBIG;
- ret = -EFBIG;
+ ret = -EFBIG;
+ if (sysnames->nr >= AFS_NR_SYSNAME)
goto out;
- }

if (strcmp(s, afs_init_sysname) == 0) {
sub = (char *)afs_init_sysname;
@@ -815,6 +825,12 @@ static ssize_t afs_proc_sysname_write(struct file *file,
inode_unlock(file_inode(file));
kfree(kbuf);
return ret;
+
+invalid:
+ ret = -EINVAL;
+error:
+ sysnames->error = ret;
+ goto out;
}

static int afs_proc_sysname_release(struct inode *inode, struct file *file)
@@ -825,14 +841,18 @@ static int afs_proc_sysname_release(struct inode *inode, struct file *file)

sysnames = m->private;
if (sysnames) {
- kill = sysnames;
if (!sysnames->error) {
+ kill = sysnames;
+ if (sysnames->nr == 0) {
+ sysnames->subs[0] = sysnames->blank;
+ sysnames->nr++;
+ }
write_lock(&net->sysnames_lock);
kill = net->sysnames;
net->sysnames = sysnames;
write_unlock(&net->sysnames_lock);
}
- afs_sysnames_free(kill);
+ afs_put_sysnames(kill);
}

return seq_release(inode, file);

Next message: Gaurav Dhingra: "Re: [PATCH v2] staging: greybus: Fix warning to limit chars per line"
Previous message: Jarkko Sakkinen: "Re: [PATCH v4 0/4] Migrate all TPM 2.0 commands to use struct tpm_buf"
In reply to: David Howells: "Re: [PATCH 05/20] afs: Implement @sys substitution handling"
Next in thread: David Howells: "[PATCH 06/20] afs: Implement @cell substitution handling"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]