[PATCH 00/13] Clean up kernel_read/kernel_write

[PATCH 00/13] Clean up kernel_read/kernel_write

[Matthew Wilcox (Oracle)]

Linus asked that NULL pos be allowed to kernel_write() / kernel_read().
This set of patches (against Al's for-next tree) does that in the first
two patches, and then converts many of the users of kernel_write() /
kernel_read() to use a NULL pointer.  I test-compiled as many as I could.

Matthew Wilcox (Oracle) (13):
  fs: Allow a NULL pos pointer to __kernel_write
  fs: Allow a NULL pos pointer to __kernel_read
  fs/acct: Pass a NULL pointer to __kernel_write
  um/mconsole: Pass a NULL pointer to kernel_read
  x86/aout: Pass a NULL pointer to kernel_read
  aout: Pass a NULL pointer to kernel_read
  binfmt_flat: Pass a NULL pointer to kernel_read
  exec: Pass a NULL pointer to kernel_read
  bpfilter: Pass a NULL pointer to kernel_read and kernel_write
  keys: Pass a NULL pointer to kernel_read and kernel_write
  target: Pass a NULL pointer to kernel_write
  proc: Pass a NULL pointer to kernel_write
  usermode: Pass a NULL pointer to kernel_write

 arch/um/drivers/mconsole_kern.c   |  3 +--
 arch/x86/ia32/ia32_aout.c         |  3 +--
 drivers/target/target_core_alua.c |  3 +--
 drivers/target/target_core_pr.c   |  3 +--
 fs/binfmt_aout.c                  |  3 +--
 fs/binfmt_flat.c                  |  3 +--
 fs/exec.c                         |  4 +---
 fs/proc/proc_sysctl.c             |  3 +--
 fs/read_write.c                   | 10 ++++++----
 kernel/acct.c                     |  4 +---
 kernel/usermode_driver.c          |  3 +--
 net/bpfilter/bpfilter_kern.c      |  7 ++-----
 security/keys/big_key.c           |  7 ++-----
 13 files changed, 20 insertions(+), 36 deletions(-)

-- 
2.28.0

[PATCH 01/13] fs: Allow a NULL pos pointer to __kernel_write

[Matthew Wilcox (Oracle)]

Linus prefers that callers be allowed to pass in a NULL pointer for ppos
like new_sync_write().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/read_write.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index d835b5c86fae..7ee07f76fafc 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -544,11 +544,12 @@ ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t
 		return warn_unsupported(file, "write");
 
 	init_sync_kiocb(&kiocb, file);
-	kiocb.ki_pos = *pos;
+	kiocb.ki_pos = pos ? *pos : 0;
 	iov_iter_kvec(&iter, WRITE, &iov, 1, iov.iov_len);
 	ret = file->f_op->write_iter(&kiocb, &iter);
 	if (ret > 0) {
-		*pos = kiocb.ki_pos;
+		if (pos)
+			*pos = kiocb.ki_pos;
 		fsnotify_modify(file);
 		add_wchar(current, ret);
 	}
-- 
2.28.0

[PATCH 02/13] fs: Allow a NULL pos pointer to __kernel_read

[Matthew Wilcox (Oracle)]

Match the behaviour of new_sync_read() and __kernel_write().

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/read_write.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 7ee07f76fafc..cf420e57541f 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -449,11 +449,12 @@ ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
 		return warn_unsupported(file, "read");
 
 	init_sync_kiocb(&kiocb, file);
-	kiocb.ki_pos = *pos;
+	kiocb.ki_pos = pos ? *pos : 0;
 	iov_iter_kvec(&iter, READ, &iov, 1, iov.iov_len);
 	ret = file->f_op->read_iter(&kiocb, &iter);
 	if (ret > 0) {
-		*pos = kiocb.ki_pos;
+		if (pos)
+			*pos = kiocb.ki_pos;
 		fsnotify_access(file);
 		add_rchar(current, ret);
 	}
-- 
2.28.0

[PATCH 03/13] fs/acct: Pass a NULL pointer to __kernel_write

[Matthew Wilcox (Oracle)]

We do nothing with the updated pos, so we can pass in NULL here.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 kernel/acct.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/acct.c b/kernel/acct.c
index b0c5b3a9f5af..08f0ec28d188 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -517,9 +517,7 @@ static void do_acct_process(struct bsd_acct_struct *acct)
 	 * as we could deadlock the system otherwise.
 	 */
 	if (file_start_write_trylock(file)) {
-		/* it's been opened O_APPEND, so position is irrelevant */
-		loff_t pos = 0;
-		__kernel_write(file, &ac, sizeof(acct_t), &pos);
+		__kernel_write(file, &ac, sizeof(acct_t), NULL);
 		file_end_write(file);
 	}
 out:
-- 
2.28.0

[PATCH 04/13] um/mconsole: Pass a NULL pointer to kernel_read

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 arch/um/drivers/mconsole_kern.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/um/drivers/mconsole_kern.c b/arch/um/drivers/mconsole_kern.c
index a2e680f7d39f..eb33464cc693 100644
--- a/arch/um/drivers/mconsole_kern.c
+++ b/arch/um/drivers/mconsole_kern.c
@@ -131,7 +131,6 @@ void mconsole_proc(struct mc_request *req)
 	struct file *file;
 	int first_chunk = 1;
 	char *ptr = req->request.data;
-	loff_t pos = 0;
 
 	ptr += strlen("proc");
 	ptr = skip_spaces(ptr);
@@ -154,7 +153,7 @@ void mconsole_proc(struct mc_request *req)
 	}
 
 	do {
-		len = kernel_read(file, buf, PAGE_SIZE - 1, &pos);
+		len = kernel_read(file, buf, PAGE_SIZE - 1, NULL);
 		if (len < 0) {
 			mconsole_reply(req, "Read of file failed", 1, 0);
 			goto out_free;
-- 
2.28.0

[PATCH 05/13] x86/aout: Pass a NULL pointer to kernel_read

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 arch/x86/ia32/ia32_aout.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index a09fc37ead9d..f2029db06e2b 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -247,10 +247,9 @@ static int load_aout_library(struct file *file)
 	unsigned long bss, start_addr, len, error;
 	int retval;
 	struct exec ex;
-	loff_t pos = 0;
 
 	retval = -ENOEXEC;
-	error = kernel_read(file, &ex, sizeof(ex), &pos);
+	error = kernel_read(file, &ex, sizeof(ex), NULL);
 	if (error != sizeof(ex))
 		goto out;
 
-- 
2.28.0

[PATCH 06/13] aout: Pass a NULL pointer to kernel_read

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/binfmt_aout.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
index 3e84e9bb9084..6e7fea50523d 100644
--- a/fs/binfmt_aout.c
+++ b/fs/binfmt_aout.c
@@ -259,12 +259,11 @@ static int load_aout_library(struct file *file)
 	unsigned long error;
 	int retval;
 	struct exec ex;
-	loff_t pos = 0;
 
 	inode = file_inode(file);
 
 	retval = -ENOEXEC;
-	error = kernel_read(file, &ex, sizeof(ex), &pos);
+	error = kernel_read(file, &ex, sizeof(ex), NULL);
 	if (error != sizeof(ex))
 		goto out;
 
-- 
2.28.0

[PATCH 07/13] binfmt_flat: Pass a NULL pointer to kernel_read

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/binfmt_flat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index f2f9086ebe98..b3f525be0a1a 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -891,7 +891,6 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
 	struct linux_binprm bprm;
 	int res;
 	char buf[16];
-	loff_t pos = 0;
 
 	memset(&bprm, 0, sizeof(bprm));
 
@@ -905,7 +904,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
 	if (IS_ERR(bprm.file))
 		return res;
 
-	res = kernel_read(bprm.file, bprm.buf, BINPRM_BUF_SIZE, &pos);
+	res = kernel_read(bprm.file, bprm.buf, BINPRM_BUF_SIZE, NULL);
 
 	if (res >= 0)
 		res = load_flat_file(&bprm, libs, id, NULL);
-- 
2.28.0

[PATCH 08/13] exec: Pass a NULL pointer to kernel_read

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/exec.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index a91003e28eaa..bb18ff89abcd 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1743,10 +1743,8 @@ static int bprm_creds_from_file(struct linux_binprm *bprm)
  */
 static int prepare_binprm(struct linux_binprm *bprm)
 {
-	loff_t pos = 0;
-
 	memset(bprm->buf, 0, BINPRM_BUF_SIZE);
-	return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos);
+	return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, NULL);
 }
 
 /*
-- 
2.28.0

[PATCH 09/13] bpfilter: Pass a NULL pointer to kernel_read and kernel_write

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 net/bpfilter/bpfilter_kern.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/bpfilter/bpfilter_kern.c b/net/bpfilter/bpfilter_kern.c
index 51a941b56ec3..bca294c9253f 100644
--- a/net/bpfilter/bpfilter_kern.c
+++ b/net/bpfilter/bpfilter_kern.c
@@ -34,21 +34,18 @@ static void __stop_umh(void)
 static int bpfilter_send_req(struct mbox_request *req)
 {
 	struct mbox_reply reply;
-	loff_t pos = 0;
 	ssize_t n;
 
 	if (!bpfilter_ops.info.tgid)
 		return -EFAULT;
-	pos = 0;
 	n = kernel_write(bpfilter_ops.info.pipe_to_umh, req, sizeof(*req),
-			   &pos);
+			   NULL);
 	if (n != sizeof(*req)) {
 		pr_err("write fail %zd\n", n);
 		goto stop;
 	}
-	pos = 0;
 	n = kernel_read(bpfilter_ops.info.pipe_from_umh, &reply, sizeof(reply),
-			&pos);
+			NULL);
 	if (n != sizeof(reply)) {
 		pr_err("read fail %zd\n", n);
 		goto stop;
-- 
2.28.0

[PATCH 10/13] keys: Pass a NULL pointer to kernel_read and kernel_write

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 security/keys/big_key.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 691347dea3c1..ffe70ff84665 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -79,8 +79,6 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		 * Since the key is random for each file, we can set the nonce
 		 * to zero, provided we never define a ->update() call.
 		 */
-		loff_t pos = 0;
-
 		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
@@ -106,7 +104,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 			goto err_enckey;
 		}
 
-		written = kernel_write(file, buf, enclen, &pos);
+		written = kernel_write(file, buf, enclen, NULL);
 		if (written != enclen) {
 			ret = written;
 			if (written >= 0)
@@ -240,7 +238,6 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
 		struct file *file;
 		u8 *buf, *enckey = (u8 *)key->payload.data[big_key_data];
 		size_t enclen = datalen + CHACHA20POLY1305_AUTHTAG_SIZE;
-		loff_t pos = 0;
 
 		buf = kvmalloc(enclen, GFP_KERNEL);
 		if (!buf)
@@ -253,7 +250,7 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
 		}
 
 		/* read file to kernel and decrypt */
-		ret = kernel_read(file, buf, enclen, &pos);
+		ret = kernel_read(file, buf, enclen, NULL);
 		if (ret != enclen) {
 			if (ret >= 0)
 				ret = -EIO;
-- 
2.28.0

[PATCH 11/13] target: Pass a NULL pointer to kernel_write

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 drivers/target/target_core_alua.c | 3 +--
 drivers/target/target_core_pr.c   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
index 6b72afee2f8b..98b702e50961 100644
--- a/drivers/target/target_core_alua.c
+++ b/drivers/target/target_core_alua.c
@@ -883,14 +883,13 @@ static int core_alua_write_tpg_metadata(
 	u32 md_buf_len)
 {
 	struct file *file = filp_open(path, O_RDWR | O_CREAT | O_TRUNC, 0600);
-	loff_t pos = 0;
 	int ret;
 
 	if (IS_ERR(file)) {
 		pr_err("filp_open(%s) for ALUA metadata failed\n", path);
 		return -ENODEV;
 	}
-	ret = kernel_write(file, md_buf, md_buf_len, &pos);
+	ret = kernel_write(file, md_buf, md_buf_len, NULL);
 	if (ret < 0)
 		pr_err("Error writing ALUA metadata file: %s\n", path);
 	fput(file);
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 8fc88654bff6..6fb9940e2e02 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -1976,7 +1976,6 @@ static int __core_scsi3_write_aptpl_to_file(
 	char *path;
 	u32 pr_aptpl_buf_len;
 	int ret;
-	loff_t pos = 0;
 
 	path = kasprintf(GFP_KERNEL, "%s/pr/aptpl_%s", db_root,
 			&wwn->unit_serial[0]);
@@ -1993,7 +1992,7 @@ static int __core_scsi3_write_aptpl_to_file(
 
 	pr_aptpl_buf_len = (strlen(buf) + 1); /* Add extra for NULL */
 
-	ret = kernel_write(file, buf, pr_aptpl_buf_len, &pos);
+	ret = kernel_write(file, buf, pr_aptpl_buf_len, NULL);
 
 	if (ret < 0)
 		pr_debug("Error writing APTPL metadata file: %s\n", path);
-- 
2.28.0

[PATCH 12/13] proc: Pass a NULL pointer to kernel_write

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/proc/proc_sysctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 317899222d7f..be9c46833aa5 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1754,7 +1754,6 @@ static int process_sysctl_arg(char *param, char *val,
 	struct file *file;
 	int len;
 	int err;
-	loff_t pos = 0;
 	ssize_t wret;
 
 	if (strncmp(param, "sysctl", sizeof("sysctl") - 1) == 0) {
@@ -1812,7 +1811,7 @@ static int process_sysctl_arg(char *param, char *val,
 		goto out;
 	}
 	len = strlen(val);
-	wret = kernel_write(file, val, len, &pos);
+	wret = kernel_write(file, val, len, NULL);
 	if (wret < 0) {
 		err = wret;
 		if (err == -EINVAL)
-- 
2.28.0

[PATCH 13/13] usermode: Pass a NULL pointer to kernel_write

[Matthew Wilcox (Oracle)]

We want to start at 0 and do not care about the updated value.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 kernel/usermode_driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/usermode_driver.c b/kernel/usermode_driver.c
index 0b35212ffc3d..ff880befc5bf 100644
--- a/kernel/usermode_driver.c
+++ b/kernel/usermode_driver.c
@@ -15,7 +15,6 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
 	struct vfsmount *mnt;
 	struct file *file;
 	ssize_t written;
-	loff_t pos = 0;
 
 	type = get_fs_type("tmpfs");
 	if (!type)
@@ -32,7 +31,7 @@ static struct vfsmount *blob_to_mnt(const void *data, size_t len, const char *na
 		return ERR_CAST(file);
 	}
 
-	written = kernel_write(file, data, len, &pos);
+	written = kernel_write(file, data, len, NULL);
 	if (written != len) {
 		int err = written;
 		if (err >= 0)
-- 
2.28.0

Re: [PATCH 00/13] Clean up kernel_read/kernel_write

[Al Viro]

On Sat, Oct 03, 2020 at 03:55:21AM +0100, Matthew Wilcox (Oracle) wrote:
> Linus asked that NULL pos be allowed to kernel_write() / kernel_read().
> This set of patches (against Al's for-next tree) does that in the first
> two patches, and then converts many of the users of kernel_write() /
> kernel_read() to use a NULL pointer.  I test-compiled as many as I could.

OK, applied, will push if it survives local beating.

Re: [PATCH 00/13] Clean up kernel_read/kernel_write

[Eric Biggers]

On Sat, Oct 03, 2020 at 04:47:56AM +0100, Al Viro wrote:
> On Sat, Oct 03, 2020 at 03:55:21AM +0100, Matthew Wilcox (Oracle) wrote:
> > Linus asked that NULL pos be allowed to kernel_write() / kernel_read().
> > This set of patches (against Al's for-next tree) does that in the first
> > two patches, and then converts many of the users of kernel_write() /
> > kernel_read() to use a NULL pointer.  I test-compiled as many as I could.
> 
> OK, applied, will push if it survives local beating.

Applied to where?

- Eric

Re: [PATCH 01/13] fs: Allow a NULL pos pointer to __kernel_write

[Christoph Hellwig]

On Sat, Oct 03, 2020 at 03:55:22AM +0100, Matthew Wilcox (Oracle) wrote:
> Linus prefers that callers be allowed to pass in a NULL pointer for ppos
> like new_sync_write().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 02/13] fs: Allow a NULL pos pointer to __kernel_read

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 00/13] Clean up kernel_read/kernel_write

[Christoph Hellwig]

Al,

can you pick up patches 1 and 2 to unbreak linux-next?

Re: [PATCH 00/13] Clean up kernel_read/kernel_write

[Al Viro]

On Thu, Oct 15, 2020 at 07:56:41PM +0200, Christoph Hellwig wrote:
> Al,
> 
> can you pick up patches 1 and 2 to unbreak linux-next?

OK... I really don't like the idea of mangling base.set_fs, so
a bit of bisect hazard will be there - I'd put these two into
a branch on top of base.set_fs (#work.set_fs).  Regenerated
with addition of your reviewed-by, added to #for-next and pushed
out.