[RFC 1/5] seq_file: introduce seq_open_data helper

[RFC 1/5] seq_file: introduce seq_open_data helper

[Rasmus Villemoes]

There are quite a few callers of seq_open that could be simplified by
setting the ->private member via the seq_open call instead of fetching
file->private_data afterwards.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
I've just included a few examples of possible users of this helper,
there are many more similar cases. As a bonus, the first two fix
potential NULL derefs (if one believes that seq_open can actually
fail).

seq_open_private would have been a better name, but that one is
already taken...

Documentation/filesystems/seq_file.txt | 9 +++++----
 fs/seq_file.c                          | 9 ++++++++-
 include/linux/seq_file.h               | 1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
index 9de4303201e1..68571b8275d8 100644
--- a/Documentation/filesystems/seq_file.txt
+++ b/Documentation/filesystems/seq_file.txt
@@ -234,10 +234,11 @@ Here, the call to seq_open() takes the seq_operations structure we created
 before, and gets set up to iterate through the virtual file.
 
 On a successful open, seq_open() stores the struct seq_file pointer in
-file->private_data. If you have an application where the same iterator can
-be used for more than one file, you can store an arbitrary pointer in the
-private field of the seq_file structure; that value can then be retrieved
-by the iterator functions.
+file->private_data. If you have an application where the same iterator
+can be used for more than one file, you can store an arbitrary pointer
+in the private field of the seq_file structure; that value can then be
+retrieved by the iterator functions. Using the wrapper seq_open_data()
+allows you to set the initial value for that field.
 
 There is also a wrapper function to seq_open() called seq_open_private(). It
 kmallocs a zero filled block of memory and stores a pointer to it in the
diff --git a/fs/seq_file.c b/fs/seq_file.c
index eea09f6d8830..f2145cb6e23d 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -45,7 +45,7 @@ static void *seq_buf_alloc(unsigned long size)
  *	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)
+int seq_open_data(struct file *file, const struct seq_operations *op, void *data)
 {
 	struct seq_file *p;
 
@@ -59,6 +59,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
 
 	mutex_init(&p->lock);
 	p->op = op;
+	p->private = data;
 
 	// No refcounting: the lifetime of 'p' is constrained
 	// to the lifetime of the file.
@@ -85,6 +86,12 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }
 EXPORT_SYMBOL(seq_open);
 
+int seq_open(struct file *file, const struct seq_operations *op)
+{
+	return seq_open_data(file, op, NULL);
+}
+EXPORT_SYMBOL(seq_open_data);
+
 static int traverse(struct seq_file *m, loff_t offset)
 {
 	loff_t pos = 0, index;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index ab437dd2e3b9..f5ff376fa62b 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -107,6 +107,7 @@ void seq_pad(struct seq_file *m, char c);
 
 char *mangle_path(char *s, const char *p, const char *esc);
 int seq_open(struct file *, const struct seq_operations *);
+int seq_open_data(struct file *, const struct seq_operations *, void *);
 ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
 loff_t seq_lseek(struct file *, loff_t, int);
 int seq_release(struct inode *, struct file *);
-- 
2.15.1

[RFC 2/5] ia64/sn/hwperf: use seq_open_data

[Rasmus Villemoes]

This code should check the return value of seq_open(); if it failed,
file->private_data is NULL. But we can avoid the issue entirely and
simplify the code by letting seq_open_data() set the ->private member
to objbuf.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 arch/ia64/sn/kernel/sn2/sn_hwperf.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/ia64/sn/kernel/sn2/sn_hwperf.c b/arch/ia64/sn/kernel/sn2/sn_hwperf.c
index 55febd65911a..fba7f3ad99f4 100644
--- a/arch/ia64/sn/kernel/sn2/sn_hwperf.c
+++ b/arch/ia64/sn/kernel/sn2/sn_hwperf.c
@@ -941,14 +941,11 @@ static int sn_hwperf_init(void)
 int sn_topology_open(struct inode *inode, struct file *file)
 {
 	int e;
-	struct seq_file *seq;
 	struct sn_hwperf_object_info *objbuf;
 	int nobj;
 
 	if ((e = sn_hwperf_enum_objects(&nobj, &objbuf)) = 0) {
-		e = seq_open(file, &sn_topology_seq_ops);
-		seq = file->private_data;
-		seq->private = objbuf;
+		e = seq_open_data(file, &sn_topology_seq_ops, objbuf);
 	}
 
 	return e;
-- 
2.15.1

Re: [RFC 2/5] ia64/sn/hwperf: use seq_open_data

[Rasmus Villemoes]

On 2 March 2018 at 00:37, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> This code should check the return value of seq_open(); if it failed,
> file->private_data is NULL. But we can avoid the issue entirely and
> simplify the code by letting seq_open_data() set the ->private member
> to objbuf.
>
>         if ((e = sn_hwperf_enum_objects(&nobj, &objbuf)) = 0) {
> -               e = seq_open(file, &sn_topology_seq_ops);
> -               seq = file->private_data;
> -               seq->private = objbuf;
> +               e = seq_open_data(file, &sn_topology_seq_ops, objbuf);
>         }

Well, this turns a NULL deref into a resource leak; we still need to
add a check of the return value and vfree(objbuf) on failure.

Rasmus

Re: [RFC 1/5] seq_file: introduce seq_open_data helper

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 4152 bytes --]

On Mar 1, 2018, at 4:37 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> There are quite a few callers of seq_open that could be simplified by
> setting the ->private member via the seq_open call instead of fetching
> file->private_data afterwards.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> I've just included a few examples of possible users of this helper,
> there are many more similar cases. As a bonus, the first two fix
> potential NULL derefs (if one believes that seq_open can actually
> fail).
> 
> seq_open_private would have been a better name, but that one is
> already taken...
> 
> Documentation/filesystems/seq_file.txt | 9 +++++----
> fs/seq_file.c                          | 9 ++++++++-
> include/linux/seq_file.h               | 1 +
> 3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/filesystems/seq_file.txt b/Documentation/filesystems/seq_file.txt
> index 9de4303201e1..68571b8275d8 100644
> --- a/Documentation/filesystems/seq_file.txt
> +++ b/Documentation/filesystems/seq_file.txt
> @@ -234,10 +234,11 @@ Here, the call to seq_open() takes the seq_operations structure we created
> before, and gets set up to iterate through the virtual file.
> 
> On a successful open, seq_open() stores the struct seq_file pointer in
> -file->private_data. If you have an application where the same iterator can
> -be used for more than one file, you can store an arbitrary pointer in the
> -private field of the seq_file structure; that value can then be retrieved
> -by the iterator functions.
> +file->private_data. If you have an application where the same iterator
> +can be used for more than one file, you can store an arbitrary pointer
> +in the private field of the seq_file structure; that value can then be
> +retrieved by the iterator functions. Using the wrapper seq_open_data()
> +allows you to set the initial value for that field.
> 
> There is also a wrapper function to seq_open() called seq_open_private(). It
> kmallocs a zero filled block of memory and stores a pointer to it in the
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index eea09f6d8830..f2145cb6e23d 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -45,7 +45,7 @@ static void *seq_buf_alloc(unsigned long size)
>  *	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)
> +int seq_open_data(struct file *file, const struct seq_operations *op, void *data)
> {
> 	struct seq_file *p;
> 
> @@ -59,6 +59,7 @@ int seq_open(struct file *file, const struct seq_operations *op)
> 
> 	mutex_init(&p->lock);
> 	p->op = op;
> +	p->private = data;
> 
> 	// No refcounting: the lifetime of 'p' is constrained
> 	// to the lifetime of the file.
> @@ -85,6 +86,12 @@ int seq_open(struct file *file, const struct seq_operations *op)
> }
> EXPORT_SYMBOL(seq_open);
> 
> +int seq_open(struct file *file, const struct seq_operations *op)
> +{
> +	return seq_open_data(file, op, NULL);
> +}
> +EXPORT_SYMBOL(seq_open_data);

This is a bit confusing.  You export "seq_open" after seq_open_data(),
and export "seq_open_data" here after seq_open().  Not strictly a bug,
but could become one in the future.

Cheers, Andreas

> +
> static int traverse(struct seq_file *m, loff_t offset)
> {
> 	loff_t pos = 0, index;
> diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
> index ab437dd2e3b9..f5ff376fa62b 100644
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -107,6 +107,7 @@ void seq_pad(struct seq_file *m, char c);
> 
> char *mangle_path(char *s, const char *p, const char *esc);
> int seq_open(struct file *, const struct seq_operations *);
> +int seq_open_data(struct file *, const struct seq_operations *, void *);
> ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
> loff_t seq_lseek(struct file *, loff_t, int);
> int seq_release(struct inode *, struct file *);
> --
> 2.15.1
> 





[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]