Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1333391 > unrolled thread

[PATCH v3 3/7] debugfs: add support for self-protecting attribute file fops

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/7] debugfs: add support for self-protecting attribute file fops Nicolai Stange <nicstange@gmail.com> - 2016-02-14 02:40 +0100
    Re: [PATCH v3 3/7] debugfs: add support for self-protecting attribute  file fops Julia Lawall <julia.lawall@lip6.fr> - 2016-02-14 08:10 +0100
      Re: [PATCH v3 3/7] debugfs: add support for self-protecting attribute file fops Nicolai Stange <nicstange@gmail.com> - 2016-02-14 09:30 +0100
        Re: [PATCH v3 3/7] debugfs: add support for self-protecting attribute  file fops Julia Lawall <julia.lawall@lip6.fr> - 2016-02-14 17:30 +0100

#1333391 — [PATCH v3 3/7] debugfs: add support for self-protecting attribute file fops

In order to protect them against file removal issues, debugfs_create_file()
creates a lifetime managing proxy around each struct file_operations
handed in.

In cases where this struct file_operations is able to manage file lifetime
by itself already, the proxy created by debugfs is a waste of resources.

The most common class of struct file_operations given to debugfs are those
defined by means of the DEFINE_SIMPLE_ATTRIBUTE() macro.

Introduce a DEFINE_DEBUGFS_ATTRIBUTE() macro to allow any
struct file_operations of this class to be easily made file lifetime aware
and thus, to be operated unproxied.

Specifically, introduce debugfs_attr_read() and debugfs_attr_write()
which wrap simple_attr_read() and simple_attr_write() under the protection
of a debugfs_use_file_start()/debugfs_use_file_finish() pair.

Make DEFINE_DEBUGFS_ATTRIBUTE() set the defined struct file_operations'
->read() and ->write() members to these wrappers.

Export debugfs_create_file_unsafe() in order to allow debugfs users to
create their files in non-proxying operation mode.

Finally, add a Coccinelle script chasing down possible candidates
for a DEFINE_SIMPLE_ATTRIBUTE()/debugfs_create_file() to
DEFINE_DEBUGFS_ATTRIBUTE()/debugfs_create_file_unsafe() migration.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c                                  | 28 +++++++++
 fs/debugfs/inode.c                                 | 28 +++++++++
 include/linux/debugfs.h                            | 26 +++++++++
 .../api/debugfs/debugfs_simple_attr.cocci          | 68 ++++++++++++++++++++++
 4 files changed, 150 insertions(+)
 create mode 100644 scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index f638dbc..2da5fb0 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -285,6 +285,34 @@ const struct file_operations debugfs_full_proxy_file_operations = {
 	.open = full_proxy_open,
 };
 
+ssize_t debugfs_attr_read(struct file *file, char __user *buf,
+			size_t len, loff_t *ppos)
+{
+	ssize_t ret;
+	int srcu_idx;
+
+	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
+	if (likely(!ret))
+		ret = simple_attr_read(file, buf, len, ppos);
+	debugfs_use_file_finish(srcu_idx);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(debugfs_attr_read);
+
+ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
+			 size_t len, loff_t *ppos)
+{
+	ssize_t ret;
+	int srcu_idx;
+
+	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
+	if (likely(!ret))
+		ret = simple_attr_write(file, buf, len, ppos);
+	debugfs_use_file_finish(srcu_idx);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(debugfs_attr_write);
+
 static struct dentry *debugfs_create_mode(const char *name, umode_t mode,
 					  struct dentry *parent, void *value,
 				          const struct file_operations *fops,
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 42a9b34..f95e355 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -368,6 +368,33 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_file);
 
+/**
+ * debugfs_create_file_unsafe - create a file in the debugfs filesystem
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have.
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @data: a pointer to something that the caller will want to get to later
+ *        on.  The inode.i_private pointer will point to this value on
+ *        the open() call.
+ * @fops: a pointer to a struct file_operations that should be used for
+ *        this file.
+ *
+ * debugfs_create_file_unsafe() is completely analogous to
+ * debugfs_create_file(), the only difference being that the fops
+ * handed it will not get protected against file removals by the
+ * debugfs core.
+ *
+ * It is your responsibility to protect your struct file_operation
+ * methods against file removals by means of debugfs_use_file_start()
+ * and debugfs_use_file_finish(). ->open() is still protected by
+ * debugfs though.
+ *
+ * Any struct file_operations defined by means of
+ * DEFINE_DEBUGFS_ATTRIBUTE() is protected against file removals and
+ * thus, may be used here.
+ */
 struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
 				   struct dentry *parent, void *data,
 				   const struct file_operations *fops)
@@ -378,6 +405,7 @@ struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
 					&debugfs_noop_file_operations,
 				fops);
 }
+EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
 
 /**
  * debugfs_create_file_size - create a file in the debugfs filesystem
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 6d45ae6..c880fe9 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -50,6 +50,9 @@ extern struct srcu_struct debugfs_srcu;
 struct dentry *debugfs_create_file(const char *name, umode_t mode,
 				   struct dentry *parent, void *data,
 				   const struct file_operations *fops);
+struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
+				   struct dentry *parent, void *data,
+				   const struct file_operations *fops);
 
 struct dentry *debugfs_create_file_size(const char *name, umode_t mode,
 					struct dentry *parent, void *data,
@@ -74,6 +77,26 @@ int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
 
 void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
 
+ssize_t debugfs_attr_read(struct file *file, char __user *buf,
+			size_t len, loff_t *ppos);
+ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
+			size_t len, loff_t *ppos);
+
+#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
+static int __fops ## _open(struct inode *inode, struct file *file)	\
+{									\
+	__simple_attr_check_format(__fmt, 0ull);			\
+	return simple_attr_open(inode, file, __get, __set, __fmt);	\
+}									\
+static const struct file_operations __fops = {				\
+	.owner	 = THIS_MODULE,					\
+	.open	 = __fops ## _open,					\
+	.release = simple_attr_release,				\
+	.read	 = debugfs_attr_read,					\
+	.write	 = debugfs_attr_write,					\
+	.llseek  = generic_file_llseek,				\
+}
+
 struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, const char *new_name);
 
@@ -183,6 +206,9 @@ static int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
 static void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
 { }
 
+#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)	\
+	static const struct file_operations __fops = { 0 }
+
 static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, char *new_name)
 {
diff --git a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
new file mode 100644
index 0000000..bdc418d
--- /dev/null
+++ b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
@@ -0,0 +1,68 @@
+///
+/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
+/// for debugfs files.
+///
+/// Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
+/// imposes some significant overhead as compared to
+/// DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
+///
+// Copyright (C): 2016 Nicolai Stange
+// Options: --no-includes
+//
+
+virtual context
+virtual patch
+virtual org
+virtual report
+
+@dsa@
+declarer name DEFINE_SIMPLE_ATTRIBUTE;
+identifier dsa_fops;
+expression dsa_get, dsa_set, dsa_fmt;
+position p;
+@@
+DEFINE_SIMPLE_ATTRIBUTE@p(dsa_fops, dsa_get, dsa_set, dsa_fmt);
+
+@dcf@
+expression name, mode, parent, data;
+identifier dsa.dsa_fops;
+@@
+debugfs_create_file(name, mode, parent, data, &dsa_fops)
+
+
+@context_dsa depends on context && dcf@
+declarer name DEFINE_DEBUGFS_ATTRIBUTE;
+identifier dsa.dsa_fops;
+expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
+@@
+* DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
+
+
+@patch_dcf depends on patch expression@
+expression name, mode, parent, data;
+identifier dsa.dsa_fops;
+@@
+- debugfs_create_file(name, mode, parent, data, &dsa_fops)
++ debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops)
+
+@patch_dsa depends on patch_dcf && patch@
+identifier dsa.dsa_fops;
+expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
+@@
+- DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
++ DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
+
+
+@script:python depends on org && dcf@
+fops << dsa.dsa_fops;
+p << dsa.p;
+@@
+msg="%s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on report && dcf@
+fops << dsa.dsa_fops;
+p << dsa.p;
+@@
+msg="WARNING: %s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
+coccilib.report.print_report(p[0], msg)
-- 
2.7.1

[toc] | [next] | [standalone]

#1333446 — Re: [PATCH v3 3/7] debugfs: add support for self-protecting attribute file fops

On Sun, 14 Feb 2016, Nicolai Stange wrote:

> In order to protect them against file removal issues, debugfs_create_file()
> creates a lifetime managing proxy around each struct file_operations
> handed in.
> 
> In cases where this struct file_operations is able to manage file lifetime
> by itself already, the proxy created by debugfs is a waste of resources.
> 
> The most common class of struct file_operations given to debugfs are those
> defined by means of the DEFINE_SIMPLE_ATTRIBUTE() macro.
> 
> Introduce a DEFINE_DEBUGFS_ATTRIBUTE() macro to allow any
> struct file_operations of this class to be easily made file lifetime aware
> and thus, to be operated unproxied.
> 
> Specifically, introduce debugfs_attr_read() and debugfs_attr_write()
> which wrap simple_attr_read() and simple_attr_write() under the protection
> of a debugfs_use_file_start()/debugfs_use_file_finish() pair.
> 
> Make DEFINE_DEBUGFS_ATTRIBUTE() set the defined struct file_operations'
> ->read() and ->write() members to these wrappers.
> 
> Export debugfs_create_file_unsafe() in order to allow debugfs users to
> create their files in non-proxying operation mode.
> 
> Finally, add a Coccinelle script chasing down possible candidates
> for a DEFINE_SIMPLE_ATTRIBUTE()/debugfs_create_file() to
> DEFINE_DEBUGFS_ATTRIBUTE()/debugfs_create_file_unsafe() migration.
> 
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
>  fs/debugfs/file.c                                  | 28 +++++++++
>  fs/debugfs/inode.c                                 | 28 +++++++++
>  include/linux/debugfs.h                            | 26 +++++++++
>  .../api/debugfs/debugfs_simple_attr.cocci          | 68 ++++++++++++++++++++++

Shouldn't the .cocci file be in a different patch, since it has a 
different maintainer?

julia

>  4 files changed, 150 insertions(+)
>  create mode 100644 scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index f638dbc..2da5fb0 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -285,6 +285,34 @@ const struct file_operations debugfs_full_proxy_file_operations = {
>  	.open = full_proxy_open,
>  };
>  
> +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
> +			size_t len, loff_t *ppos)
> +{
> +	ssize_t ret;
> +	int srcu_idx;
> +
> +	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
> +	if (likely(!ret))
> +		ret = simple_attr_read(file, buf, len, ppos);
> +	debugfs_use_file_finish(srcu_idx);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(debugfs_attr_read);
> +
> +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
> +			 size_t len, loff_t *ppos)
> +{
> +	ssize_t ret;
> +	int srcu_idx;
> +
> +	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
> +	if (likely(!ret))
> +		ret = simple_attr_write(file, buf, len, ppos);
> +	debugfs_use_file_finish(srcu_idx);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(debugfs_attr_write);
> +
>  static struct dentry *debugfs_create_mode(const char *name, umode_t mode,
>  					  struct dentry *parent, void *value,
>  				          const struct file_operations *fops,
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 42a9b34..f95e355 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -368,6 +368,33 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_file);
>  
> +/**
> + * debugfs_create_file_unsafe - create a file in the debugfs filesystem
> + * @name: a pointer to a string containing the name of the file to create.
> + * @mode: the permission that the file should have.
> + * @parent: a pointer to the parent dentry for this file.  This should be a
> + *          directory dentry if set.  If this parameter is NULL, then the
> + *          file will be created in the root of the debugfs filesystem.
> + * @data: a pointer to something that the caller will want to get to later
> + *        on.  The inode.i_private pointer will point to this value on
> + *        the open() call.
> + * @fops: a pointer to a struct file_operations that should be used for
> + *        this file.
> + *
> + * debugfs_create_file_unsafe() is completely analogous to
> + * debugfs_create_file(), the only difference being that the fops
> + * handed it will not get protected against file removals by the
> + * debugfs core.
> + *
> + * It is your responsibility to protect your struct file_operation
> + * methods against file removals by means of debugfs_use_file_start()
> + * and debugfs_use_file_finish(). ->open() is still protected by
> + * debugfs though.
> + *
> + * Any struct file_operations defined by means of
> + * DEFINE_DEBUGFS_ATTRIBUTE() is protected against file removals and
> + * thus, may be used here.
> + */
>  struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
>  				   struct dentry *parent, void *data,
>  				   const struct file_operations *fops)
> @@ -378,6 +405,7 @@ struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
>  					&debugfs_noop_file_operations,
>  				fops);
>  }
> +EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
>  
>  /**
>   * debugfs_create_file_size - create a file in the debugfs filesystem
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 6d45ae6..c880fe9 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -50,6 +50,9 @@ extern struct srcu_struct debugfs_srcu;
>  struct dentry *debugfs_create_file(const char *name, umode_t mode,
>  				   struct dentry *parent, void *data,
>  				   const struct file_operations *fops);
> +struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
> +				   struct dentry *parent, void *data,
> +				   const struct file_operations *fops);
>  
>  struct dentry *debugfs_create_file_size(const char *name, umode_t mode,
>  					struct dentry *parent, void *data,
> @@ -74,6 +77,26 @@ int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
>  
>  void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
>  
> +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
> +			size_t len, loff_t *ppos);
> +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
> +			size_t len, loff_t *ppos);
> +
> +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
> +static int __fops ## _open(struct inode *inode, struct file *file)	\
> +{									\
> +	__simple_attr_check_format(__fmt, 0ull);			\
> +	return simple_attr_open(inode, file, __get, __set, __fmt);	\
> +}									\
> +static const struct file_operations __fops = {				\
> +	.owner	 = THIS_MODULE,					\
> +	.open	 = __fops ## _open,					\
> +	.release = simple_attr_release,				\
> +	.read	 = debugfs_attr_read,					\
> +	.write	 = debugfs_attr_write,					\
> +	.llseek  = generic_file_llseek,				\
> +}
> +
>  struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
>                  struct dentry *new_dir, const char *new_name);
>  
> @@ -183,6 +206,9 @@ static int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
>  static void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
>  { }
>  
> +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)	\
> +	static const struct file_operations __fops = { 0 }
> +
>  static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
>                  struct dentry *new_dir, char *new_name)
>  {
> diff --git a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> new file mode 100644
> index 0000000..bdc418d
> --- /dev/null
> +++ b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> @@ -0,0 +1,68 @@
> +///
> +/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
> +/// for debugfs files.
> +///
> +/// Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
> +/// imposes some significant overhead as compared to
> +/// DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
> +///
> +// Copyright (C): 2016 Nicolai Stange
> +// Options: --no-includes
> +//
> +
> +virtual context
> +virtual patch
> +virtual org
> +virtual report
> +
> +@dsa@
> +declarer name DEFINE_SIMPLE_ATTRIBUTE;
> +identifier dsa_fops;
> +expression dsa_get, dsa_set, dsa_fmt;
> +position p;
> +@@
> +DEFINE_SIMPLE_ATTRIBUTE@p(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> +
> +@dcf@
> +expression name, mode, parent, data;
> +identifier dsa.dsa_fops;
> +@@
> +debugfs_create_file(name, mode, parent, data, &dsa_fops)
> +
> +
> +@context_dsa depends on context && dcf@
> +declarer name DEFINE_DEBUGFS_ATTRIBUTE;
> +identifier dsa.dsa_fops;
> +expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
> +@@
> +* DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> +
> +
> +@patch_dcf depends on patch expression@
> +expression name, mode, parent, data;
> +identifier dsa.dsa_fops;
> +@@
> +- debugfs_create_file(name, mode, parent, data, &dsa_fops)
> ++ debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops)
> +
> +@patch_dsa depends on patch_dcf && patch@
> +identifier dsa.dsa_fops;
> +expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
> +@@
> +- DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> ++ DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> +
> +
> +@script:python depends on org && dcf@
> +fops << dsa.dsa_fops;
> +p << dsa.p;
> +@@
> +msg="%s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
> +coccilib.org.print_todo(p[0], msg)
> +
> +@script:python depends on report && dcf@
> +fops << dsa.dsa_fops;
> +p << dsa.p;
> +@@
> +msg="WARNING: %s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
> +coccilib.report.print_report(p[0], msg)
> -- 
> 2.7.1
> 
> 

[toc] | [prev] | [next] | [standalone]

#1333458

Julia Lawall <julia.lawall@lip6.fr> writes:

> On Sun, 14 Feb 2016, Nicolai Stange wrote:
>
>> In order to protect them against file removal issues, debugfs_create_file()
>> creates a lifetime managing proxy around each struct file_operations
>> handed in.
>> 
>> In cases where this struct file_operations is able to manage file lifetime
>> by itself already, the proxy created by debugfs is a waste of resources.
>> 
>> The most common class of struct file_operations given to debugfs are those
>> defined by means of the DEFINE_SIMPLE_ATTRIBUTE() macro.
>> 
>> Introduce a DEFINE_DEBUGFS_ATTRIBUTE() macro to allow any
>> struct file_operations of this class to be easily made file lifetime aware
>> and thus, to be operated unproxied.
>> 
>> Specifically, introduce debugfs_attr_read() and debugfs_attr_write()
>> which wrap simple_attr_read() and simple_attr_write() under the protection
>> of a debugfs_use_file_start()/debugfs_use_file_finish() pair.
>> 
>> Make DEFINE_DEBUGFS_ATTRIBUTE() set the defined struct file_operations'
>> ->read() and ->write() members to these wrappers.
>> 
>> Export debugfs_create_file_unsafe() in order to allow debugfs users to
>> create their files in non-proxying operation mode.
>> 
>> Finally, add a Coccinelle script chasing down possible candidates
>> for a DEFINE_SIMPLE_ATTRIBUTE()/debugfs_create_file() to
>> DEFINE_DEBUGFS_ATTRIBUTE()/debugfs_create_file_unsafe() migration.
>> 
>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>> ---
>>  fs/debugfs/file.c                                  | 28 +++++++++
>>  fs/debugfs/inode.c                                 | 28 +++++++++
>>  include/linux/debugfs.h                            | 26 +++++++++
>>  .../api/debugfs/debugfs_simple_attr.cocci          | 68 ++++++++++++++++++++++
>
> Shouldn't the .cocci file be in a different patch, since it has a 
> different maintainer?

Certainly. I'll split this off in v4. Before resending I'll wait for
other reviews though.

Is the .cocci file itself Ok in that it matches the expected
style/conventions?

Thank you,

Nicolai

>>  4 files changed, 150 insertions(+)
>>  create mode 100644 scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>> 
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index f638dbc..2da5fb0 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -285,6 +285,34 @@ const struct file_operations debugfs_full_proxy_file_operations = {
>>  	.open = full_proxy_open,
>>  };
>>  
>> +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
>> +			size_t len, loff_t *ppos)
>> +{
>> +	ssize_t ret;
>> +	int srcu_idx;
>> +
>> +	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
>> +	if (likely(!ret))
>> +		ret = simple_attr_read(file, buf, len, ppos);
>> +	debugfs_use_file_finish(srcu_idx);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(debugfs_attr_read);
>> +
>> +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
>> +			 size_t len, loff_t *ppos)
>> +{
>> +	ssize_t ret;
>> +	int srcu_idx;
>> +
>> +	ret = debugfs_use_file_start(F_DENTRY(file), &srcu_idx);
>> +	if (likely(!ret))
>> +		ret = simple_attr_write(file, buf, len, ppos);
>> +	debugfs_use_file_finish(srcu_idx);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(debugfs_attr_write);
>> +
>>  static struct dentry *debugfs_create_mode(const char *name, umode_t mode,
>>  					  struct dentry *parent, void *value,
>>  				          const struct file_operations *fops,
>> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
>> index 42a9b34..f95e355 100644
>> --- a/fs/debugfs/inode.c
>> +++ b/fs/debugfs/inode.c
>> @@ -368,6 +368,33 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
>>  }
>>  EXPORT_SYMBOL_GPL(debugfs_create_file);
>>  
>> +/**
>> + * debugfs_create_file_unsafe - create a file in the debugfs filesystem
>> + * @name: a pointer to a string containing the name of the file to create.
>> + * @mode: the permission that the file should have.
>> + * @parent: a pointer to the parent dentry for this file.  This should be a
>> + *          directory dentry if set.  If this parameter is NULL, then the
>> + *          file will be created in the root of the debugfs filesystem.
>> + * @data: a pointer to something that the caller will want to get to later
>> + *        on.  The inode.i_private pointer will point to this value on
>> + *        the open() call.
>> + * @fops: a pointer to a struct file_operations that should be used for
>> + *        this file.
>> + *
>> + * debugfs_create_file_unsafe() is completely analogous to
>> + * debugfs_create_file(), the only difference being that the fops
>> + * handed it will not get protected against file removals by the
>> + * debugfs core.
>> + *
>> + * It is your responsibility to protect your struct file_operation
>> + * methods against file removals by means of debugfs_use_file_start()
>> + * and debugfs_use_file_finish(). ->open() is still protected by
>> + * debugfs though.
>> + *
>> + * Any struct file_operations defined by means of
>> + * DEFINE_DEBUGFS_ATTRIBUTE() is protected against file removals and
>> + * thus, may be used here.
>> + */
>>  struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
>>  				   struct dentry *parent, void *data,
>>  				   const struct file_operations *fops)
>> @@ -378,6 +405,7 @@ struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
>>  					&debugfs_noop_file_operations,
>>  				fops);
>>  }
>> +EXPORT_SYMBOL_GPL(debugfs_create_file_unsafe);
>>  
>>  /**
>>   * debugfs_create_file_size - create a file in the debugfs filesystem
>> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
>> index 6d45ae6..c880fe9 100644
>> --- a/include/linux/debugfs.h
>> +++ b/include/linux/debugfs.h
>> @@ -50,6 +50,9 @@ extern struct srcu_struct debugfs_srcu;
>>  struct dentry *debugfs_create_file(const char *name, umode_t mode,
>>  				   struct dentry *parent, void *data,
>>  				   const struct file_operations *fops);
>> +struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
>> +				   struct dentry *parent, void *data,
>> +				   const struct file_operations *fops);
>>  
>>  struct dentry *debugfs_create_file_size(const char *name, umode_t mode,
>>  					struct dentry *parent, void *data,
>> @@ -74,6 +77,26 @@ int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
>>  
>>  void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
>>  
>> +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
>> +			size_t len, loff_t *ppos);
>> +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
>> +			size_t len, loff_t *ppos);
>> +
>> +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
>> +static int __fops ## _open(struct inode *inode, struct file *file)	\
>> +{									\
>> +	__simple_attr_check_format(__fmt, 0ull);			\
>> +	return simple_attr_open(inode, file, __get, __set, __fmt);	\
>> +}									\
>> +static const struct file_operations __fops = {				\
>> +	.owner	 = THIS_MODULE,					\
>> +	.open	 = __fops ## _open,					\
>> +	.release = simple_attr_release,				\
>> +	.read	 = debugfs_attr_read,					\
>> +	.write	 = debugfs_attr_write,					\
>> +	.llseek  = generic_file_llseek,				\
>> +}
>> +
>>  struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
>>                  struct dentry *new_dir, const char *new_name);
>>  
>> @@ -183,6 +206,9 @@ static int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
>>  static void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
>>  { }
>>  
>> +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)	\
>> +	static const struct file_operations __fops = { 0 }
>> +
>>  static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
>>                  struct dentry *new_dir, char *new_name)
>>  {
>> diff --git a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>> new file mode 100644
>> index 0000000..bdc418d
>> --- /dev/null
>> +++ b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>> @@ -0,0 +1,68 @@
>> +///
>> +/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
>> +/// for debugfs files.
>> +///
>> +/// Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
>> +/// imposes some significant overhead as compared to
>> +/// DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
>> +///
>> +// Copyright (C): 2016 Nicolai Stange
>> +// Options: --no-includes
>> +//
>> +
>> +virtual context
>> +virtual patch
>> +virtual org
>> +virtual report
>> +
>> +@dsa@
>> +declarer name DEFINE_SIMPLE_ATTRIBUTE;
>> +identifier dsa_fops;
>> +expression dsa_get, dsa_set, dsa_fmt;
>> +position p;
>> +@@
>> +DEFINE_SIMPLE_ATTRIBUTE@p(dsa_fops, dsa_get, dsa_set, dsa_fmt);
>> +
>> +@dcf@
>> +expression name, mode, parent, data;
>> +identifier dsa.dsa_fops;
>> +@@
>> +debugfs_create_file(name, mode, parent, data, &dsa_fops)
>> +
>> +
>> +@context_dsa depends on context && dcf@
>> +declarer name DEFINE_DEBUGFS_ATTRIBUTE;
>> +identifier dsa.dsa_fops;
>> +expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
>> +@@
>> +* DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
>> +
>> +
>> +@patch_dcf depends on patch expression@
>> +expression name, mode, parent, data;
>> +identifier dsa.dsa_fops;
>> +@@
>> +- debugfs_create_file(name, mode, parent, data, &dsa_fops)
>> ++ debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops)
>> +
>> +@patch_dsa depends on patch_dcf && patch@
>> +identifier dsa.dsa_fops;
>> +expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
>> +@@
>> +- DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
>> ++ DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
>> +
>> +
>> +@script:python depends on org && dcf@
>> +fops << dsa.dsa_fops;
>> +p << dsa.p;
>> +@@
>> +msg="%s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
>> +coccilib.org.print_todo(p[0], msg)
>> +
>> +@script:python depends on report && dcf@
>> +fops << dsa.dsa_fops;
>> +p << dsa.p;
>> +@@
>> +msg="WARNING: %s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
>> +coccilib.report.print_report(p[0], msg)
>> -- 
>> 2.7.1
>> 
>> 

[toc] | [prev] | [next] | [standalone]

#1333514 — Re: [PATCH v3 3/7] debugfs: add support for self-protecting attribute file fops

> >> diff --git a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> >> new file mode 100644
> >> index 0000000..bdc418d
> >> --- /dev/null
> >> +++ b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> >> @@ -0,0 +1,68 @@
> >> +///

Could you drop the above line?

> >> +/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
> >> +/// for debugfs files.
> >> +///
> >> +/// Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
> >> +/// imposes some significant overhead as compared to
> >> +/// DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().

For the above three lines that give more detail, please use //#

thanks,
julia

> >> +// Copyright (C): 2016 Nicolai Stange
> >> +// Options: --no-includes
> >> +//
> >> +
> >> +virtual context
> >> +virtual patch
> >> +virtual org
> >> +virtual report
> >> +
> >> +@dsa@
> >> +declarer name DEFINE_SIMPLE_ATTRIBUTE;
> >> +identifier dsa_fops;
> >> +expression dsa_get, dsa_set, dsa_fmt;
> >> +position p;
> >> +@@
> >> +DEFINE_SIMPLE_ATTRIBUTE@p(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> >> +
> >> +@dcf@
> >> +expression name, mode, parent, data;
> >> +identifier dsa.dsa_fops;
> >> +@@
> >> +debugfs_create_file(name, mode, parent, data, &dsa_fops)
> >> +
> >> +
> >> +@context_dsa depends on context && dcf@
> >> +declarer name DEFINE_DEBUGFS_ATTRIBUTE;
> >> +identifier dsa.dsa_fops;
> >> +expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
> >> +@@
> >> +* DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> >> +
> >> +
> >> +@patch_dcf depends on patch expression@
> >> +expression name, mode, parent, data;
> >> +identifier dsa.dsa_fops;
> >> +@@
> >> +- debugfs_create_file(name, mode, parent, data, &dsa_fops)
> >> ++ debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops)
> >> +
> >> +@patch_dsa depends on patch_dcf && patch@
> >> +identifier dsa.dsa_fops;
> >> +expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
> >> +@@
> >> +- DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> >> ++ DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> >> +
> >> +
> >> +@script:python depends on org && dcf@
> >> +fops << dsa.dsa_fops;
> >> +p << dsa.p;
> >> +@@
> >> +msg="%s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
> >> +coccilib.org.print_todo(p[0], msg)
> >> +
> >> +@script:python depends on report && dcf@
> >> +fops << dsa.dsa_fops;
> >> +p << dsa.p;
> >> +@@
> >> +msg="WARNING: %s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
> >> +coccilib.report.print_report(p[0], msg)
> >> -- 
> >> 2.7.1
> >> 
> >> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web