sysfs: use transient write buffer

There isn't much to be gained by keeping around kernel buffer while a
file is open especially as the read path planned to be converted to
use seq_file and won't use the buffer.  This patch makes
sysfs_write_file() use per-write transient buffer instead of
sysfs_open_file->page.

This simplifies the write path, enables removing sysfs_open_file->page
once read path is updated and will help merging bin file write path
which already requires the use of a transient buffer due to a locking
order issue.

As the function comments of flush_write_buffer() and
sysfs_write_buffer() are being updated anyway, reformat them so that
they're more conventional.

v2: Use min_t() instead of min() in sysfs_write_file() to avoid build
    warning on arm.  Reported by build test robot.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index af6e9092a67997ba8962219e56cf97f1a682489b..53cc096e6a1bd67230a8b99645c78d89edd88e65 100644 (file)
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -162,92 +162,82 @@ out:
 }
 
 /**
- *     fill_write_buffer - copy buffer from userspace.
- *     @of:            open file struct.
- *     @buf:           data from user.
- *     @count:         number of bytes in @userbuf.
+ * flush_write_buffer - push buffer to kobject
+ * @of: open file
+ * @buf: data buffer for file
+ * @count: number of bytes
  *
- *     Allocate @of->page if it hasn't been already, then copy the
- *     user-supplied buffer into it.
+ * Get the correct pointers for the kobject and the attribute we're dealing
+ * with, then call the store() method for it with @buf.
  */
-static int fill_write_buffer(struct sysfs_open_file *of,
-                            const char __user *buf, size_t count)
-{
-       int error;
-
-       if (!of->page)
-               of->page = (char *)get_zeroed_page(GFP_KERNEL);
-       if (!of->page)
-               return -ENOMEM;
-
-       if (count >= PAGE_SIZE)
-               count = PAGE_SIZE - 1;
-       error = copy_from_user(of->page, buf, count);
-
-       /*
-        * If buf is assumed to contain a string, terminate it by \0, so
-        * e.g. sscanf() can scan the string easily.
-        */
-       of->page[count] = 0;
-       return error ? -EFAULT : count;
-}
-
-/**
- *     flush_write_buffer - push buffer to kobject.
- *     @of:            open file
- *     @count:         number of bytes
- *
- *     Get the correct pointers for the kobject and the attribute we're
- *     dealing with, then call the store() method for the attribute,
- *     passing the buffer that we acquired in fill_write_buffer().
- */
-static int flush_write_buffer(struct sysfs_open_file *of, size_t count)
+static int flush_write_buffer(struct sysfs_open_file *of, char *buf,
+                             size_t count)
 {
        struct kobject *kobj = of->sd->s_parent->s_dir.kobj;
        const struct sysfs_ops *ops;
-       int rc;
+       int rc = 0;
 
-       /* need @of->sd for attr and ops, its parent for kobj */
-       if (!sysfs_get_active(of->sd))
+       /*
+        * Need @of->sd for attr and ops, its parent for kobj.  @of->mutex
+        * nests outside active ref and is just to ensure that the ops
+        * aren't called concurrently for the same open file.
+        */
+       mutex_lock(&of->mutex);
+       if (!sysfs_get_active(of->sd)) {
+               mutex_unlock(&of->mutex);
                return -ENODEV;
+       }
 
        ops = sysfs_file_ops(of->sd);
-       rc = ops->store(kobj, of->sd->s_attr.attr, of->page, count);
+       rc = ops->store(kobj, of->sd->s_attr.attr, buf, count);
 
        sysfs_put_active(of->sd);
+       mutex_unlock(&of->mutex);
 
        return rc;
 }
 
 /**
- *     sysfs_write_file - write an attribute.
- *     @file:  file pointer
- *     @buf:   data to write
- *     @count: number of bytes
- *     @ppos:  starting offset
+ * sysfs_write_file - write an attribute
+ * @file: file pointer
+ * @user_buf: data to write
+ * @count: number of bytes
+ * @ppos: starting offset
  *
- *     Similar to sysfs_read_file(), though working in the opposite direction.
- *     We allocate and fill the data from the user in fill_write_buffer(),
- *     then push it to the kobject in flush_write_buffer().
- *     There is no easy way for us to know if userspace is only doing a partial
- *     write, so we don't support them. We expect the entire buffer to come
- *     on the first write.
- *     Hint: if you're writing a value, first read the file, modify only the
- *     the value you're changing, then write entire buffer back.
+ * Copy data in from userland and pass it to the matching
+ * sysfs_ops->store() by invoking flush_write_buffer().
+ *
+ * There is no easy way for us to know if userspace is only doing a partial
+ * write, so we don't support them. We expect the entire buffer to come on
+ * the first write.  Hint: if you're writing a value, first read the file,
+ * modify only the the value you're changing, then write entire buffer
+ * back.
  */
-static ssize_t sysfs_write_file(struct file *file, const char __user *buf,
+static ssize_t sysfs_write_file(struct file *file, const char __user *user_buf,
                                size_t count, loff_t *ppos)
 {
        struct sysfs_open_file *of = file->private_data;
-       ssize_t len;
+       ssize_t len = min_t(size_t, count, PAGE_SIZE - 1);
+       char *buf;
 
-       mutex_lock(&of->mutex);
-       len = fill_write_buffer(of, buf, count);
-       if (len > 0)
-               len = flush_write_buffer(of, len);
+       if (!len)
+               return 0;
+
+       buf = kmalloc(len + 1, GFP_KERNEL);
+       if (!buf)
+               return -ENOMEM;
+
+       if (copy_from_user(buf, user_buf, len)) {
+               len = -EFAULT;
+               goto out_free;
+       }
+       buf[len] = '\0';        /* guarantee string termination */
+
+       len = flush_write_buffer(of, buf, len);
        if (len > 0)
                *ppos += len;
-       mutex_unlock(&of->mutex);
+out_free:
+       kfree(buf);
        return len;
 }