This patch ensures that the inodes allocated by the functions get_sb_pseudo
and simple_fill_super are unique, provided of course, that the filesystems
calling them play by the rules. Currently that isn't the case, but will be
as I get to each of the filesystems.
The patch itself is pretty simple, but I also had to fix up the callers of
simple_fill_super to make sure they passed in the seq flag. For this first
pass, I set it on any filesystem with an empty "files" struct to 0. That may
need to be revised as I review each filesystem, but should be OK for now.
Signed-off-by: Jeff Layton <[email protected]>
diff --git a/drivers/infiniband/hw/ipath/ipath_fs.c b/drivers/infiniband/hw/ipath/ipath_fs.c
index d9ff283..c127995 100644
--- a/drivers/infiniband/hw/ipath/ipath_fs.c
+++ b/drivers/infiniband/hw/ipath/ipath_fs.c
@@ -514,7 +514,7 @@ static int ipathfs_fill_super(struct sup
{""},
};
- ret = simple_fill_super(sb, IPATHFS_MAGIC, files);
+ ret = simple_fill_super(sb, IPATHFS_MAGIC, files, 1);
if (ret) {
printk(KERN_ERR "simple_fill_super failed: %d\n", ret);
goto bail;
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 1713c48..7a90112 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -731,7 +731,7 @@ static int bm_fill_super(struct super_bl
[2] = {"register", &bm_register_operations, S_IWUSR},
/* last one */ {""}
};
- int err = simple_fill_super(sb, 0x42494e4d, bm_files);
+ int err = simple_fill_super(sb, 0x42494e4d, bm_files, 1);
if (!err)
sb->s_op = &s_ops;
return err;
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 137d76c..58becfe 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -107,7 +107,7 @@ static int debug_fill_super(struct super
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files, 0);
}
static int debug_get_sb(struct file_system_type *fs_type,
diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index 16b39c0..b14daf7 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -163,7 +163,7 @@ static int fuse_ctl_fill_super(struct su
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr, 0);
if (err)
return err;
diff --git a/fs/libfs.c b/fs/libfs.c
index bd08e0e..4fa7487 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -215,7 +215,7 @@ int get_sb_pseudo(struct file_system_typ
s->s_op = ops ? ops : &default_ops;
s->s_time_gran = 1;
root = new_inode(s);
- if (!root)
+ if (!root || iunique_register(root, 0))
goto Enomem;
root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
root->i_uid = root->i_gid = 0;
@@ -356,7 +356,12 @@ int simple_commit_write(struct file *fil
return 0;
}
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+/*
+ * Some filesystems require that particular entries have particular i_ino values. Those
+ * callers need to set the "seq" flag to make sure that i_ino is assigned sequentially
+ * to the files starting with 0.
+ */
+int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files, int seq)
{
static struct super_operations s_ops = {.statfs = simple_statfs};
struct inode *inode;
@@ -380,6 +385,9 @@ int simple_fill_super(struct super_block
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
inode->i_nlink = 2;
+ /* set this as high as a 32 bit val as possible to avoid collisions. This is also
+ * well above the highest value that iunique_register will assign to an inode */
+ inode->i_ino = 0xffffffff;
root = d_alloc_root(inode);
if (!root) {
iput(inode);
@@ -399,7 +407,10 @@ int simple_fill_super(struct super_block
inode->i_blocks = 0;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_fop = files->ops;
- inode->i_ino = i;
+ if (seq)
+ inode->i_ino = i;
+ else if (iunique_register(inode, 0))
+ goto out;
d_add(dentry, inode);
}
s->s_root = root;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 39aed90..9c4f585 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -659,7 +659,7 @@ #ifdef CONFIG_NFSD_V4
#endif
/* last one */ {""}
};
- return simple_fill_super(sb, 0x6e667364, nfsd_files);
+ return simple_fill_super(sb, 0x6e667364, nfsd_files, 1);
}
static int nfsd_get_sb(struct file_system_type *fs_type,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 48c416e..5c4513b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1890,7 +1890,7 @@ extern const struct file_operations simp
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int simple_fill_super(struct super_block *, int, struct tree_descr *, int);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
diff --git a/security/inode.c b/security/inode.c
index 9b16e14..76005f5 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -130,7 +130,7 @@ static int fill_super(struct super_block
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return simple_fill_super(sb, SECURITYFS_MAGIC, files, 0);
}
static int get_sb(struct file_system_type *fs_type,
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index cd24441..ceb4a8e 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1285,7 +1285,7 @@ static int sel_fill_super(struct super_b
[SEL_COMPAT_NET] = {"compat_net", &sel_compat_net_ops, S_IRUGO|S_IWUSR},
/* last one */ {""}
};
- ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files);
+ ret = simple_fill_super(sb, SELINUX_MAGIC, selinux_files, 1);
if (ret)
goto err;
Josef Sipek wrote:
>> - ret = simple_fill_super(sb, IPATHFS_MAGIC, files);
>> + ret = simple_fill_super(sb, IPATHFS_MAGIC, files, 1);
>
> I don't know...the magic looking 1 and 0 (later in the patch) seem a bit
> arbitrary. Maybe a #define is in order?
Yeah, I'm not fond of that, though the comments on simple_fill_super should
explain it. Basically, I need simple_fill_super to operate in two different
"modes", and I was using the extra flag to key this. I'm not clear on what
sort of #define would make sense here. Can you suggest something?
>> -int simple_fill_super(struct super_block *s, int magic, struct tree_descr
>> *files)
>> +/*
>> + * Some filesystems require that particular entries have particular i_ino
>> values. Those
>> + * callers need to set the "seq" flag to make sure that i_ino is assigned
>> sequentially
>> + * to the files starting with 0.
>> + */
>> +int simple_fill_super(struct super_block *s, int magic, struct tree_descr
>> *files, int seq)
>
> Line wraped.
>
I thought those were under 80 columns but perhaps they weren't. I'll clean
them up and repost, once you clarify what you'd like to see in the #define.
>> @@ -399,7 +407,10 @@ int simple_fill_super(struct super_block
>> inode->i_blocks = 0;
>> inode->i_atime = inode->i_mtime = inode->i_ctime =
>> CURRENT_TIME;
>
> I'd indent CURRENT_TIME a bit.
I wasn't planning on touching those parts of the code that don't need to be
changed, since formatting deltas can make it harder to see the "actual"
changes in the patch. That should probably be addressed in a follow-on
patch if you think it needs to be changed.
-- Jeff Layton
On Fri, Dec 08, 2006 at 08:08:03AM -0500, Jeff Layton wrote:
> Josef Sipek wrote:
> >> - ret = simple_fill_super(sb, IPATHFS_MAGIC, files);
> >> + ret = simple_fill_super(sb, IPATHFS_MAGIC, files, 1);
> >
> > I don't know...the magic looking 1 and 0 (later in the patch) seem a bit
> > arbitrary. Maybe a #define is in order?
>
> Yeah, I'm not fond of that, though the comments on simple_fill_super should
> explain it. Basically, I need simple_fill_super to operate in two different
> "modes", and I was using the extra flag to key this. I'm not clear on what
> sort of #define would make sense here. Can you suggest something?
First I was thinking about defining 2 constats, but maybe the better thing
to do would be to
1) rename simple_fill_super to __simple_fill_super
2) #define simple_fill_super_foo(...) to __siple_fill_super(....., 0)
3) #define simple_fill_super_bar(...) to __siple_fill_super(....., 1)
(Or equivalent thing using inline functions.)
I can't really think of any good name for #define'd flag.
Beware, I'm pretty much just thinking out loud.. :)
> >> @@ -399,7 +407,10 @@ int simple_fill_super(struct super_block
> >> inode->i_blocks = 0;
> >> inode->i_atime = inode->i_mtime = inode->i_ctime =
> >> CURRENT_TIME;
> >
> > I'd indent CURRENT_TIME a bit.
>
> I wasn't planning on touching those parts of the code that don't need to be
> changed, since formatting deltas can make it harder to see the "actual"
> changes in the patch. That should probably be addressed in a follow-on
> patch if you think it needs to be changed.
Oh, sorry, that wasn't your code. You're right about it not being the the
right thing to fix in your patch.
Actually, it looks like another problem created by line-wrapping.
Josef "Jeff" Sipek.
--
NT is to UNIX what a doughnut is to a particle accelerator.
On Thu, Dec 07, 2006 at 05:13:08PM -0500, Jeff Layton wrote:
> This patch ensures that the inodes allocated by the functions get_sb_pseudo
> and simple_fill_super are unique, provided of course, that the filesystems
> calling them play by the rules. Currently that isn't the case, but will be
> as I get to each of the filesystems.
>
> The patch itself is pretty simple, but I also had to fix up the callers of
> simple_fill_super to make sure they passed in the seq flag. For this first
> pass, I set it on any filesystem with an empty "files" struct to 0. That may
> need to be revised as I review each filesystem, but should be OK for now.
>
> Signed-off-by: Jeff Layton <[email protected]>
>
> diff --git a/drivers/infiniband/hw/ipath/ipath_fs.c
> b/drivers/infiniband/hw/ipath/ipath_fs.c
> index d9ff283..c127995 100644
> --- a/drivers/infiniband/hw/ipath/ipath_fs.c
> +++ b/drivers/infiniband/hw/ipath/ipath_fs.c
> @@ -514,7 +514,7 @@ static int ipathfs_fill_super(struct sup
> {""},
> };
>
> - ret = simple_fill_super(sb, IPATHFS_MAGIC, files);
> + ret = simple_fill_super(sb, IPATHFS_MAGIC, files, 1);
I don't know...the magic looking 1 and 0 (later in the patch) seem a bit
arbitrary. Maybe a #define is in order?
> -int simple_fill_super(struct super_block *s, int magic, struct tree_descr
> *files)
> +/*
> + * Some filesystems require that particular entries have particular i_ino
> values. Those
> + * callers need to set the "seq" flag to make sure that i_ino is assigned
> sequentially
> + * to the files starting with 0.
> + */
> +int simple_fill_super(struct super_block *s, int magic, struct tree_descr
> *files, int seq)
Line wraped.
> @@ -399,7 +407,10 @@ int simple_fill_super(struct super_block
> inode->i_blocks = 0;
> inode->i_atime = inode->i_mtime = inode->i_ctime =
> CURRENT_TIME;
I'd indent CURRENT_TIME a bit.
Josef "Jeff" Sipek.
--
If I have trouble installing Linux, something is wrong. Very wrong.
- Linus Torvalds
Jeff Layton wrote:
> This patch ensures that the inodes allocated by the functions get_sb_pseudo
> and simple_fill_super are unique, provided of course, that the filesystems
> calling them play by the rules. Currently that isn't the case, but will be
> as I get to each of the filesystems.
>
This patch is respun based on some suggestions by Josef Sipek. The different
modes of simple_fill_super are different enough that they probably ought to
be different functions. This makes that so.
With this patch it's not necessary to change some of the callers of
simple_fill_super yet, but they'll need to be changed sometime so I left
that in place.
Signed-off-by: Jeff Layton <[email protected]>
diff --git a/drivers/infiniband/hw/ipath/ipath_fs.c b/drivers/infiniband/hw/ipath/ipath_fs.c
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 137d76c..3f9dd93 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -107,7 +107,7 @@ static int debug_fill_super(struct super
{
static struct tree_descr debug_files[] = {{""}};
- return simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
+ return registered_fill_super(sb, DEBUGFS_MAGIC, debug_files);
}
static int debug_get_sb(struct file_system_type *fs_type,
diff --git a/fs/fuse/control.c b/fs/fuse/control.c
index 8c58bd4..1320066 100644
--- a/fs/fuse/control.c
+++ b/fs/fuse/control.c
@@ -163,7 +163,7 @@ static int fuse_ctl_fill_super(struct su
struct fuse_conn *fc;
int err;
- err = simple_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
+ err = registered_fill_super(sb, FUSE_CTL_SUPER_MAGIC, &empty_descr);
if (err)
return err;
diff --git a/fs/libfs.c b/fs/libfs.c
index 503898d..ba1a887 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -215,7 +215,7 @@ int get_sb_pseudo(struct file_system_typ
s->s_op = ops ? ops : &default_ops;
s->s_time_gran = 1;
root = new_inode(s);
- if (!root)
+ if (!root || iunique_register(root, 0))
goto Enomem;
root->i_mode = S_IFDIR | S_IRUSR | S_IWUSR;
root->i_uid = root->i_gid = 0;
@@ -356,7 +356,8 @@ int simple_commit_write(struct file *fil
return 0;
}
-int simple_fill_super(struct super_block *s, int magic, struct tree_descr *files)
+static int __simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files, int sequential)
{
static struct super_operations s_ops = {.statfs = simple_statfs};
struct inode *inode;
@@ -380,6 +381,12 @@ int simple_fill_super(struct super_block
inode->i_op = &simple_dir_inode_operations;
inode->i_fop = &simple_dir_operations;
inode->i_nlink = 2;
+ /*
+ * set this as high as a 32 bit val as possible to avoid collisions.
+ * This is also well above the highest value that iunique_register
+ * will assign to an inode
+ */
+ inode->i_ino = 0xffffffff;
root = d_alloc_root(inode);
if (!root) {
iput(inode);
@@ -394,12 +401,15 @@ int simple_fill_super(struct super_block
inode = new_inode(s);
if (!inode)
goto out;
+ if (sequential)
+ inode->i_ino = i;
+ else if (iunique_register(inode, 0))
+ goto out;
inode->i_mode = S_IFREG | files->mode;
inode->i_uid = inode->i_gid = 0;
inode->i_blocks = 0;
inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
inode->i_fop = files->ops;
- inode->i_ino = i;
d_add(dentry, inode);
}
s->s_root = root;
@@ -410,6 +420,30 @@ out:
return -ENOMEM;
}
+/*
+ * Fill a superblock with a standard set of fields, and add the entries in the
+ * "files" struct. Assign i_ino values to the files sequentially. This function
+ * is appropriate for filesystems that need a particular i_ino value assigned
+ * to a particular "files" entry.
+ */
+int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, 1);
+}
+
+/*
+ * Just like simple_fill_super, but does an iunique_register on the inodes
+ * created for "files" entries. This function is appropriate when you don't
+ * need a particular i_ino value assigned to each files entry, and when the
+ * filesystem will have other registered inodes.
+ */
+int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files)
+{
+ return __simple_fill_super(s, magic, files, 0);
+}
+
static DEFINE_SPINLOCK(pin_fs_lock);
int simple_pin_fs(struct file_system_type *type, struct vfsmount **mount, int *count)
@@ -619,6 +653,7 @@ EXPORT_SYMBOL(simple_dir_operations);
EXPORT_SYMBOL(simple_empty);
EXPORT_SYMBOL(d_alloc_name);
EXPORT_SYMBOL(simple_fill_super);
+EXPORT_SYMBOL(registered_fill_super);
EXPORT_SYMBOL(simple_getattr);
EXPORT_SYMBOL(simple_link);
EXPORT_SYMBOL(simple_lookup);
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6c70f1a..c98bc80 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1879,7 +1879,10 @@ extern const struct file_operations simp
extern struct inode_operations simple_dir_inode_operations;
struct tree_descr { char *name; const struct file_operations *ops; int mode; };
struct dentry *d_alloc_name(struct dentry *, const char *);
-extern int simple_fill_super(struct super_block *, int, struct tree_descr *);
+extern int simple_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files);
+extern int registered_fill_super(struct super_block *s, int magic,
+ struct tree_descr *files);
extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
extern void simple_release_fs(struct vfsmount **mount, int *count);
diff --git a/security/inode.c b/security/inode.c
index 9b16e14..e3e12b1 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -130,7 +130,7 @@ static int fill_super(struct super_block
{
static struct tree_descr files[] = {{""}};
- return simple_fill_super(sb, SECURITYFS_MAGIC, files);
+ return registered_fill_super(sb, SECURITYFS_MAGIC, files);
}
static int get_sb(struct file_system_type *fs_type,
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c