No more seq_file pre-allocation

PATCH New Yann Droneaud 1 participant 4 messages Jun 1, 2015 VFS

lore

Share & tools

lore

Yann Droneaud

author

2015-06-01 21:32 UTC #0 lore raw

cover No more seq_file pre-allocation

Hi,

Please find a revised patchset to remove support for passing pre-allocated struct seq_file to seq_open().

Such feature is undocumented and prone to error.

In particular, if seq_release() is used in release handler, it will kfree() a pointer which was not allocated by seq_open().

So, please find a patchset that drop the support for pre-allocated struct seq_file: it's only of use in proc_namespace.c and can be easily replaced by using seq_open_private()/seq_release_private().

Additionally, it documents the use of file->private_data to hold pointer to struct seq_file by seq_open().

Changes from v0 [0]:
- convert kmalloc() + memset() to kzalloc()
- revised a bit commit messages

[0] [PATCH 0/3] seq_file allocation in seq_open()

http://lkml.kernel.org/r/[email protected]
http://lkml.kernel.org/g/[email protected]

Yann Droneaud (3):
fs: use seq_open_private() for proc_mounts
fs: allocate structure unconditionally in seq_open()
fs: documents seq_open()'s usage of file->private_data

 fs/mount.h          |  3 ---
 fs/namespace.c      |  6 +++---
 fs/proc_namespace.c | 34 ++++++++++++++++------------------
 fs/seq_file.c       | 19 +++++++++++--------
 4 files changed, 30 insertions(+), 32 deletions(-)

Signature · 1 line

2.4.2

Yann Droneaud

author

2015-06-01 21:32 UTC #1 lore raw

PATCH 1/3 fs: use seq_open_private() for proc_mounts

Since patch described below, from v2.6.15-rc1, seq_open() could use a struct seq_file already allocated by the caller if the pointer to the structure is stored in file->private_data before calling the function.

Commit 1abe77b0fc4b485927f1f798ae81a752677e1d05
Author: Al Viro <[email protected]>
Date: Mon Nov 7 17:15:34 2005 -0500

[PATCH] allow callers of seq_open do allocation themselves

Allow caller of seq_open() to kmalloc() seq_file + whatever else they want and set ->private_data to it. seq_open() will then abstain from doing allocation itself.

Such behavior is only used by mounts_open_common().

In order to drop support for such uncommon feature, proc_mounts is converted to use seq_open_private(), which take care of allocating the proc_mounts structure, making it available through ->private in struct seq_file.

Conversely, proc_mounts is converted to use seq_release_private(), in order to release the private structure allocated by seq_open_private().

Then, ->private is used directly instead of proc_mounts() macro to access to the proc_mounts structure.

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Yann Droneaud <[email protected]>

---
 fs/mount.h          |  3 ---
 fs/namespace.c      |  6 +++---
 fs/proc_namespace.c | 34 ++++++++++++++++------------------
 3 files changed, 19 insertions(+), 24 deletions(-)

fs/mount.h +0 -3

index b5b8082bfa42..14db05d424f7 100644
@@ -118,7 +118,6 @@ static inline void unlock_mount_hash(void)
 }
 
 struct proc_mounts {
-	struct seq_file m;
 	struct mnt_namespace *ns;
 	struct path root;
 	int (*show)(struct seq_file *, struct vfsmount *);

@@ -127,8 +126,6 @@ struct proc_mounts {
 	loff_t cached_index;
 };
 
-#define proc_mounts(p) (container_of((p), struct proc_mounts, m))
-
 extern const struct seq_operations mounts_op;
 
 extern bool __is_local_mountpoint(struct dentry *dentry);

fs/namespace.c +3 -3

index f30c78a2b878..ecc277107e93 100644
@@ -1226,7 +1226,7 @@ EXPORT_SYMBOL(replace_mount_options);
 /* iterator; we want it to have access to namespace_sem, thus here... */
 static void *m_start(struct seq_file *m, loff_t *pos)
 {
-	struct proc_mounts *p = proc_mounts(m);
+	struct proc_mounts *p = m->private;
 
 	down_read(&namespace_sem);
 	if (p->cached_event == p->ns->event) {

@@ -1247,7 +1247,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 
 static void *m_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	struct proc_mounts *p = proc_mounts(m);
+	struct proc_mounts *p = m->private;
 
 	p->cached_mount = seq_list_next(v, &p->ns->list, pos);
 	p->cached_index = *pos;

@@ -1261,7 +1261,7 @@ static void m_stop(struct seq_file *m, void *v)
 
 static int m_show(struct seq_file *m, void *v)
 {
-	struct proc_mounts *p = proc_mounts(m);
+	struct proc_mounts *p = m->private;
 	struct mount *r = list_entry(v, struct mount, mnt_list);
 	return p->show(m, &r->mnt);
 }

fs/proc_namespace.c +16 ~~-18~~

index 8db932da4009..8ebd9a334085 100644
@@ -17,7 +17,8 @@
 
 static unsigned mounts_poll(struct file *file, poll_table *wait)
 {
-	struct proc_mounts *p = proc_mounts(file->private_data);
+	struct seq_file *m = file->private_data;
+	struct proc_mounts *p = m->private;
 	struct mnt_namespace *ns = p->ns;
 	unsigned res = POLLIN | POLLRDNORM;
 	int event;

@@ -25,8 +26,8 @@ static unsigned mounts_poll(struct file *file, poll_table *wait)
 	poll_wait(file, &p->ns->poll, wait);
 
 	event = ACCESS_ONCE(ns->event);
-	if (p->m.poll_event != event) {
-		p->m.poll_event = event;
+	if (m->poll_event != event) {
+		m->poll_event = event;
 		res |= POLLERR | POLLPRI;
 	}

@@ -92,7 +93,7 @@ static void show_type(struct seq_file *m, struct super_block *sb)
 
 static int show_vfsmnt(struct seq_file *m, struct vfsmount *mnt)
 {
-	struct proc_mounts *p = proc_mounts(m);
+	struct proc_mounts *p = m->private;
 	struct mount *r = real_mount(mnt);
 	int err = 0;
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };

@@ -126,7 +127,7 @@ out:
 
 static int show_mountinfo(struct seq_file *m, struct vfsmount *mnt)
 {
-	struct proc_mounts *p = proc_mounts(m);
+	struct proc_mounts *p = m->private;
 	struct mount *r = real_mount(mnt);
 	struct super_block *sb = mnt->mnt_sb;
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };

@@ -186,7 +187,7 @@ out:
 
 static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
 {
-	struct proc_mounts *p = proc_mounts(m);
+	struct proc_mounts *p = m->private;
 	struct mount *r = real_mount(mnt);
 	struct path mnt_path = { .dentry = mnt->mnt_root, .mnt = mnt };
 	struct super_block *sb = mnt_path.dentry->d_sb;

@@ -236,6 +237,7 @@ static int mounts_open_common(struct inode *inode, struct file *file,
 	struct mnt_namespace *ns = NULL;
 	struct path root;
 	struct proc_mounts *p;
+	struct seq_file *m;
 	int ret = -EINVAL;
 
 	if (!task)

@@ -260,26 +262,21 @@ static int mounts_open_common(struct inode *inode, struct file *file,
 	task_unlock(task);
 	put_task_struct(task);
 
-	ret = -ENOMEM;
-	p = kmalloc(sizeof(struct proc_mounts), GFP_KERNEL);
-	if (!p)
+	ret = seq_open_private(file, &mounts_op, sizeof(struct proc_mounts));
+	if (ret)
 		goto err_put_path;
 
-	file->private_data = &p->m;
-	ret = seq_open(file, &mounts_op);
-	if (ret)
-		goto err_free;
+	m = file->private_data;
+	m->poll_event = ns->event;
 
+	p = m->private;
 	p->ns = ns;
 	p->root = root;
-	p->m.poll_event = ns->event;
 	p->show = show;
 	p->cached_event = ~0ULL;
 
 	return 0;
 
- err_free:
-	kfree(p);
  err_put_path:
 	path_put(&root);
  err_put_ns:

@@ -290,10 +287,11 @@ static int mounts_open_common(struct inode *inode, struct file *file,
 
 static int mounts_release(struct inode *inode, struct file *file)
 {
-	struct proc_mounts *p = proc_mounts(file->private_data);
+	struct seq_file *m = file->private_data;
+	struct proc_mounts *p = m->private;
 	path_put(&p->root);
 	put_mnt_ns(p->ns);
-	return seq_release(inode, file);
+	return seq_release_private(inode, file);
 }
 
 static int mounts_open(struct inode *inode, struct file *file)

Signature · 1 line

2.4.2

Yann Droneaud

author

2015-06-01 21:32 UTC #2 lore raw

PATCH 2/3 fs: allocate structure unconditionally in seq_open()

In reply to Yann Droneaud

Commit 1abe77b0fc4b485927f1f798ae81a752677e1d05
Author: Al Viro <[email protected]>
Date: Mon Nov 7 17:15:34 2005 -0500

[PATCH] allow callers of seq_open do allocation themselves

Allow caller of seq_open() to kmalloc() seq_file + whatever else they want and set ->private_data to it. seq_open() will then abstain from doing allocation itself.

As there's no more use for such feature, as it could be easily replaced by calls to seq_open_private() (see commit 39699037a5c9 ("[FS] seq_file: Introduce the seq_open_private()")) and seq_release_private() (see v2.6.0-test3), support for this uncommon feature can be removed from seq_open().

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Yann Droneaud <[email protected]>

---
 fs/seq_file.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

fs/seq_file.c +9 -8

index c14f6a43beb5..a909f12dad4d 100644
@@ -51,15 +51,16 @@ static void *seq_buf_alloc(unsigned long size)
  */
 int seq_open(struct file *file, const struct seq_operations *op)
 {
-	struct seq_file *p = file->private_data;
+	struct seq_file *p;
+
+	WARN_ON(file->private_data);
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	file->private_data = p;
 
-	if (!p) {
-		p = kmalloc(sizeof(*p), GFP_KERNEL);
-		if (!p)
-			return -ENOMEM;
-		file->private_data = p;
-	}
-	memset(p, 0, sizeof(*p));
 	mutex_init(&p->lock);
 	p->op = op;
 #ifdef CONFIG_USER_NS

Signature · 1 line

2.4.2

Yann Droneaud

author

2015-06-01 21:32 UTC #3 lore raw

PATCH 3/3 fs: documents seq_open()'s usage of file->private_data

In reply to Yann Droneaud

seq_open() store its struct seq_file in file->private_data, thus, it must not be modified by user of seq_file.

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Yann Droneaud <[email protected]>

---
 fs/seq_file.c | 2 ++
 1 file changed, 2 insertions(+)

fs/seq_file.c +2 -0

index a909f12dad4d..e48ef5682bfa 100644
@@ -48,6 +48,8 @@ static void *seq_buf_alloc(unsigned long size)
  *	ERR_PTR(error).  In the end of sequence they return %NULL. ->show()
  *	returns 0 in case of success and negative number in case of error.
  *	Returning SEQ_SKIP means "discard this element and move on".
+ *	Note: seq_open() will allocate a struct seq_file and store its
+ *	pointer in @file->private_data. This pointer should not be modified.
  */
 int seq_open(struct file *file, const struct seq_operations *op)
 {

Signature · 1 line

2.4.2