From dd33f1f7aaf0ce4d0b5ca139019ffbe3510238eb Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Tue, 25 May 2021 10:24:23 +0200 Subject: [PATCH 1/5] configfs: fix the kerneldoc comment for configfs_create_bin_file Mention the correct argument name. Signed-off-by: Bart Van Assche [hch: split from a larger patch] Signed-off-by: Christoph Hellwig --- fs/configfs/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/configfs/file.c b/fs/configfs/file.c index e26060dae70a..e557b8806b58 100644 --- a/fs/configfs/file.c +++ b/fs/configfs/file.c @@ -532,7 +532,7 @@ int configfs_create_file(struct config_item * item, const struct configfs_attrib /** * configfs_create_bin_file - create a binary attribute file for an item. * @item: item we're creating for. - * @attr: atrribute descriptor. + * @bin_attr: atrribute descriptor. */ int configfs_create_bin_file(struct config_item *item, From 44b9a000df5cc144c262a0da0d092dc04b93abca Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Tue, 25 May 2021 10:28:54 +0200 Subject: [PATCH 2/5] configfs: drop pointless kerneldoc comments file.c has a bunch of kerneldoc comments for static functions that do not document any API but just list what is done. Drop them. Signed-off-by: Christoph Hellwig --- fs/configfs/file.c | 85 ++++------------------------------------------ 1 file changed, 6 insertions(+), 79 deletions(-) diff --git a/fs/configfs/file.c b/fs/configfs/file.c index e557b8806b58..2199690b4080 100644 --- a/fs/configfs/file.c +++ b/fs/configfs/file.c @@ -77,25 +77,6 @@ static int fill_read_buffer(struct file *file, struct configfs_buffer *buffer) return 0; } -/** - * configfs_read_file - read an attribute. - * @file: file pointer. - * @buf: buffer to fill. - * @count: number of bytes to read. - * @ppos: starting offset in file. - * - * Userspace wants to read an attribute file. The attribute descriptor - * is in the file's ->d_fsdata. The target item is in the directory's - * ->d_fsdata. - * - * We call fill_read_buffer() to allocate and fill the buffer from the - * item's show() method exactly once (if the read is happening from - * the beginning of the file). That should fill the entire buffer with - * all the data the item has to offer for that attribute. - * We then call flush_read_buffer() to copy the buffer to userspace - * in the increments specified. - */ - static ssize_t configfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) { @@ -117,26 +98,6 @@ configfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *pp return retval; } -/** - * configfs_read_bin_file - read a binary attribute. - * @file: file pointer. - * @buf: buffer to fill. - * @count: number of bytes to read. - * @ppos: starting offset in file. - * - * Userspace wants to read a binary attribute file. The attribute - * descriptor is in the file's ->d_fsdata. The target item is in the - * directory's ->d_fsdata. - * - * We check whether we need to refill the buffer. If so we will - * call the attributes' attr->read() twice. The first time we - * will pass a NULL as a buffer pointer, which the attributes' method - * will use to return the size of the buffer required. If no error - * occurs we will allocate the buffer using vmalloc and call - * attr->read() again passing that buffer as an argument. - * Then we just copy to user-space using simple_read_from_buffer. - */ - static ssize_t configfs_read_bin_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) @@ -207,17 +168,6 @@ configfs_read_bin_file(struct file *file, char __user *buf, return retval; } - -/** - * fill_write_buffer - copy buffer from userspace. - * @buffer: data buffer for file. - * @buf: data from user. - * @count: number of bytes in @userbuf. - * - * Allocate @buffer->page if it hasn't been already, then - * copy the user-supplied buffer into it. - */ - static int fill_write_buffer(struct configfs_buffer * buffer, const char __user * buf, size_t count) { @@ -252,23 +202,13 @@ flush_write_buffer(struct file *file, struct configfs_buffer *buffer, size_t cou } -/** - * configfs_write_file - write an attribute. - * @file: file pointer - * @buf: data to write - * @count: number of bytes - * @ppos: starting offset - * - * Similar to configfs_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 config_item 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 value you're changing, then write entire buffer back. +/* + * 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 value + * you're changing, then write entire buffer back. */ - static ssize_t configfs_write_file(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { @@ -285,19 +225,6 @@ configfs_write_file(struct file *file, const char __user *buf, size_t count, lof return len; } -/** - * configfs_write_bin_file - write a binary attribute. - * @file: file pointer - * @buf: data to write - * @count: number of bytes - * @ppos: starting offset - * - * Writing to a binary attribute file is similar to a normal read. - * We buffer the consecutive writes (binary attribute files do not - * support lseek) in a continuously growing buffer, but we don't - * commit until the close of the file. - */ - static ssize_t configfs_write_bin_file(struct file *file, const char __user *buf, size_t count, loff_t *ppos) From 7fe1e79b59ba02fb6bdc45d324f88f1ba97d3ab4 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Tue, 25 May 2021 10:30:21 +0200 Subject: [PATCH 3/5] configfs: implement the .read_iter and .write_iter methods Configfs is one of the few filesystems that does not yet support the .read_iter and .write_iter callbacks. This patch adds support for these methods in configfs. Signed-off-by: Bart Van Assche [hch: split out a separate fix] Signed-off-by: Christoph Hellwig --- fs/configfs/file.c | 84 +++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/fs/configfs/file.c b/fs/configfs/file.c index 2199690b4080..53913b84383a 100644 --- a/fs/configfs/file.c +++ b/fs/configfs/file.c @@ -14,7 +14,7 @@ #include #include #include - +#include #include #include "configfs_internal.h" @@ -77,9 +77,9 @@ static int fill_read_buffer(struct file *file, struct configfs_buffer *buffer) return 0; } -static ssize_t -configfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) +static ssize_t configfs_read_iter(struct kiocb *iocb, struct iov_iter *to) { + struct file *file = iocb->ki_filp; struct configfs_buffer *buffer = file->private_data; ssize_t retval = 0; @@ -89,23 +89,24 @@ configfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *pp if (retval) goto out; } - pr_debug("%s: count = %zd, ppos = %lld, buf = %s\n", - __func__, count, *ppos, buffer->page); - retval = simple_read_from_buffer(buf, count, ppos, buffer->page, - buffer->count); + pr_debug("%s: count = %zd, pos = %lld, buf = %s\n", + __func__, iov_iter_count(to), iocb->ki_pos, buffer->page); + retval = copy_to_iter(buffer->page, buffer->count, to); + iocb->ki_pos += retval; + if (retval == 0) + retval = -EFAULT; out: mutex_unlock(&buffer->mutex); return retval; } -static ssize_t -configfs_read_bin_file(struct file *file, char __user *buf, - size_t count, loff_t *ppos) +static ssize_t configfs_bin_read_iter(struct kiocb *iocb, struct iov_iter *to) { + struct file *file = iocb->ki_filp; struct configfs_fragment *frag = to_frag(file); struct configfs_buffer *buffer = file->private_data; ssize_t retval = 0; - ssize_t len = min_t(size_t, count, PAGE_SIZE); + ssize_t len; mutex_lock(&buffer->mutex); @@ -161,31 +162,31 @@ configfs_read_bin_file(struct file *file, char __user *buf, buffer->needs_read_fill = 0; } - retval = simple_read_from_buffer(buf, count, ppos, buffer->bin_buffer, - buffer->bin_buffer_size); + retval = copy_to_iter(buffer->bin_buffer, buffer->bin_buffer_size, to); + iocb->ki_pos += retval; + if (retval == 0) + retval = -EFAULT; out: mutex_unlock(&buffer->mutex); return retval; } -static int -fill_write_buffer(struct configfs_buffer * buffer, const char __user * buf, size_t count) +static int fill_write_buffer(struct configfs_buffer *buffer, + struct iov_iter *from) { - int error; + int copied; if (!buffer->page) buffer->page = (char *)__get_free_pages(GFP_KERNEL, 0); if (!buffer->page) return -ENOMEM; - if (count >= SIMPLE_ATTR_SIZE) - count = SIMPLE_ATTR_SIZE - 1; - error = copy_from_user(buffer->page,buf,count); + copied = copy_from_iter(buffer->page, SIMPLE_ATTR_SIZE - 1, from); buffer->needs_read_fill = 1; /* if buf is assumed to contain a string, terminate it by \0, * so e.g. sscanf() can scan the string easily */ - buffer->page[count] = 0; - return error ? -EFAULT : count; + buffer->page[copied] = 0; + return copied ? : -EFAULT; } static int @@ -209,28 +210,29 @@ flush_write_buffer(struct file *file, struct configfs_buffer *buffer, size_t cou * Hint: if you're writing a value, first read the file, modify only the value * you're changing, then write entire buffer back. */ -static ssize_t -configfs_write_file(struct file *file, const char __user *buf, size_t count, loff_t *ppos) +static ssize_t configfs_write_iter(struct kiocb *iocb, struct iov_iter *from) { + struct file *file = iocb->ki_filp; struct configfs_buffer *buffer = file->private_data; ssize_t len; mutex_lock(&buffer->mutex); - len = fill_write_buffer(buffer, buf, count); + len = fill_write_buffer(buffer, from); if (len > 0) len = flush_write_buffer(file, buffer, len); if (len > 0) - *ppos += len; + iocb->ki_pos += len; mutex_unlock(&buffer->mutex); return len; } -static ssize_t -configfs_write_bin_file(struct file *file, const char __user *buf, - size_t count, loff_t *ppos) +static ssize_t configfs_bin_write_iter(struct kiocb *iocb, + struct iov_iter *from) { + struct file *file = iocb->ki_filp; struct configfs_buffer *buffer = file->private_data; void *tbuf = NULL; + size_t end_offset; ssize_t len; mutex_lock(&buffer->mutex); @@ -243,15 +245,14 @@ configfs_write_bin_file(struct file *file, const char __user *buf, buffer->write_in_progress = true; /* buffer grows? */ - if (*ppos + count > buffer->bin_buffer_size) { - - if (buffer->cb_max_size && - *ppos + count > buffer->cb_max_size) { + end_offset = iocb->ki_pos + iov_iter_count(from); + if (end_offset > buffer->bin_buffer_size) { + if (buffer->cb_max_size && end_offset > buffer->cb_max_size) { len = -EFBIG; goto out; } - tbuf = vmalloc(*ppos + count); + tbuf = vmalloc(end_offset); if (tbuf == NULL) { len = -ENOMEM; goto out; @@ -266,16 +267,15 @@ configfs_write_bin_file(struct file *file, const char __user *buf, /* clear the new area */ memset(tbuf + buffer->bin_buffer_size, 0, - *ppos + count - buffer->bin_buffer_size); + end_offset - buffer->bin_buffer_size); buffer->bin_buffer = tbuf; - buffer->bin_buffer_size = *ppos + count; + buffer->bin_buffer_size = end_offset; } - len = simple_write_to_buffer(buffer->bin_buffer, - buffer->bin_buffer_size, ppos, buf, count); + len = copy_from_iter(buffer->bin_buffer, buffer->bin_buffer_size, from); out: mutex_unlock(&buffer->mutex); - return len; + return len ? : -EFAULT; } static int __configfs_open_file(struct inode *inode, struct file *file, int type) @@ -420,16 +420,16 @@ static int configfs_release_bin_file(struct inode *inode, struct file *file) const struct file_operations configfs_file_operations = { - .read = configfs_read_file, - .write = configfs_write_file, + .read_iter = configfs_read_iter, + .write_iter = configfs_write_iter, .llseek = generic_file_llseek, .open = configfs_open_file, .release = configfs_release, }; const struct file_operations configfs_bin_file_operations = { - .read = configfs_read_bin_file, - .write = configfs_write_bin_file, + .read_iter = configfs_bin_read_iter, + .write_iter = configfs_bin_write_iter, .llseek = NULL, /* bin file is not seekable */ .open = configfs_open_bin_file, .release = configfs_release_bin_file, From 3c252b087de08d3cb32468b54a158bd7ad0ae2f7 Mon Sep 17 00:00:00 2001 From: Chung-Chiang Cheng Date: Fri, 18 Jun 2021 15:59:25 +0800 Subject: [PATCH 4/5] configfs: fix memleak in configfs_release_bin_file When reading binary attributes in progress, buffer->bin_buffer is setup in configfs_read_bin_file() but never freed. Fixes: 03607ace807b4 ("configfs: implement binary attributes") Signed-off-by: Chung-Chiang Cheng [hch: move the vfree rather than duplicating it] Signed-off-by: Christoph Hellwig --- fs/configfs/file.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fs/configfs/file.c b/fs/configfs/file.c index 53913b84383a..396418333513 100644 --- a/fs/configfs/file.c +++ b/fs/configfs/file.c @@ -407,13 +407,13 @@ static int configfs_release_bin_file(struct inode *inode, struct file *file) buffer->bin_buffer_size); } up_read(&frag->frag_sem); - /* vfree on NULL is safe */ - vfree(buffer->bin_buffer); - buffer->bin_buffer = NULL; - buffer->bin_buffer_size = 0; - buffer->needs_read_fill = 1; } + vfree(buffer->bin_buffer); + buffer->bin_buffer = NULL; + buffer->bin_buffer_size = 0; + buffer->needs_read_fill = 1; + configfs_release(inode, file); return 0; } From c886fa3cf6ffbe13006053ceb27c93d41928de30 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Tue, 22 Jun 2021 09:43:56 +0200 Subject: [PATCH 5/5] configfs: simplify configfs_release_bin_file Remove the clearing of various fields just before freeing the buffer structure. Signed-off-by: Christoph Hellwig --- fs/configfs/file.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/fs/configfs/file.c b/fs/configfs/file.c index 396418333513..2f63bf3a7325 100644 --- a/fs/configfs/file.c +++ b/fs/configfs/file.c @@ -393,11 +393,8 @@ static int configfs_release_bin_file(struct inode *inode, struct file *file) { struct configfs_buffer *buffer = file->private_data; - buffer->read_in_progress = false; - if (buffer->write_in_progress) { struct configfs_fragment *frag = to_frag(file); - buffer->write_in_progress = false; down_read(&frag->frag_sem); if (!frag->frag_dead) { @@ -410,9 +407,6 @@ static int configfs_release_bin_file(struct inode *inode, struct file *file) } vfree(buffer->bin_buffer); - buffer->bin_buffer = NULL; - buffer->bin_buffer_size = 0; - buffer->needs_read_fill = 1; configfs_release(inode, file); return 0;