mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements
@ 2019-01-23  1:13 Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 1/7] commands: Move mem_parse_options() to lib/misc.c Andrey Smirnov
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

Everyone:

This series is a result of my attempt to fix a regression in lseek()
on 32-bit platforms. The regression manifested in lseek() unable to
seek past ~4GiB mark caused and was caused by usage of IS_ERR_VALUE()
(see commit messages for more detailed explanation). My goal was to
both get rid of IS_ERR_VALUE() while still make it possible to access
all 64-bits of address space via /dev/mem on 64-bit machines.

First three commits are optional and can be dropped.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (7):
  commands: Move mem_parse_options() to lib/misc.c
  commands: Get rid of mem_rw_buf
  commands: Move /dev/mem driver to drivers/misc
  fs: Change error checking logic for fsdrv->lseek() call
  fs: Calculate new position before validtiy check in lseek()
  fs: Add support for files larger than MAX_LFS_FILESIZE
  misc: mem: Set correct size for /dev/mem

 commands/Kconfig      |  17 +++---
 commands/Makefile     |   1 -
 commands/md.c         |  12 +++--
 commands/mem.c        | 123 ------------------------------------------
 commands/memcmp.c     |  16 +++---
 commands/memcpy.c     |  10 ++--
 commands/memset.c     |   2 -
 common/ratp/md.c      |   9 ++--
 drivers/misc/Kconfig  |   3 ++
 drivers/misc/Makefile |   1 +
 drivers/misc/mem.c    |  45 ++++++++++++++++
 fs/devfs.c            |   2 +
 fs/fs.c               |  48 +++++++++++------
 include/driver.h      |   1 +
 include/fs.h          |   1 +
 lib/misc.c            |  42 +++++++++++++++
 16 files changed, 160 insertions(+), 173 deletions(-)
 delete mode 100644 commands/mem.c
 create mode 100644 drivers/misc/mem.c

-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/7] commands: Move mem_parse_options() to lib/misc.c
  2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
@ 2019-01-23  1:13 ` Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 2/7] commands: Get rid of mem_rw_buf Andrey Smirnov
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

As a first step of de-cluttering /dev/mem related code, move
mem_parse_options() out of commands/mem.c into lib/misc.c where it
seem to fit better. With this change we no longer explicitly turn this
code off using CONFIG_COMPILE_MEMORY and instead rely on LTO to get
rid of it when it's not being used.

While at it, also fix return value by replacing COMMAND_ERROR_USAGE
with -EINVAL. All of the callers of mem_parse_options() expect
negative error code as a sign of failure and COMMAND_ERROR_USAGE is
not negative.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 commands/mem.c | 40 ----------------------------------------
 lib/misc.c     | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/commands/mem.c b/commands/mem.c
index a9e12f3e5..62488bf52 100644
--- a/commands/mem.c
+++ b/commands/mem.c
@@ -41,46 +41,6 @@
 
 char *mem_rw_buf;
 
-/*
- * Common function for parsing options for the 'md', 'mw', 'memcpy', 'memcmp'
- * commands.
- */
-int mem_parse_options(int argc, char *argv[], char *optstr, int *mode,
-		char **sourcefile, char **destfile, int *swab)
-{
-	int opt;
-
-	while((opt = getopt(argc, argv, optstr)) > 0) {
-		switch(opt) {
-		case 'b':
-			*mode = O_RWSIZE_1;
-			break;
-		case 'w':
-			*mode = O_RWSIZE_2;
-			break;
-		case 'l':
-			*mode = O_RWSIZE_4;
-			break;
-		case 'q':
-			*mode = O_RWSIZE_8;
-			break;
-		case 's':
-			*sourcefile = optarg;
-			break;
-		case 'd':
-			*destfile = optarg;
-			break;
-		case 'x':
-			*swab = 1;
-			break;
-		default:
-			return COMMAND_ERROR_USAGE;
-		}
-	}
-
-	return 0;
-}
-
 static struct cdev_operations memops = {
 	.read  = mem_read,
 	.write = mem_write,
diff --git a/lib/misc.c b/lib/misc.c
index 1d20e1b09..cd420a57d 100644
--- a/lib/misc.c
+++ b/lib/misc.c
@@ -23,6 +23,7 @@
 #include <fs.h>
 #include <string.h>
 #include <linux/ctype.h>
+#include <getopt.h>
 
 /*
  * Like simple_strtoull() but handles an optional G, M, K or k
@@ -129,3 +130,44 @@ success:
 	return 0;
 }
 EXPORT_SYMBOL(parse_area_spec);
+
+/*
+ * Common function for parsing options for the 'md', 'mw', 'memcpy', 'memcmp'
+ * commands.
+ */
+int mem_parse_options(int argc, char *argv[], char *optstr, int *mode,
+		      char **sourcefile, char **destfile, int *swab)
+{
+	int opt;
+
+	while((opt = getopt(argc, argv, optstr)) > 0) {
+		switch(opt) {
+		case 'b':
+			*mode = O_RWSIZE_1;
+			break;
+		case 'w':
+			*mode = O_RWSIZE_2;
+			break;
+		case 'l':
+			*mode = O_RWSIZE_4;
+			break;
+		case 'q':
+			*mode = O_RWSIZE_8;
+			break;
+		case 's':
+			*sourcefile = optarg;
+			break;
+		case 'd':
+			*destfile = optarg;
+			break;
+		case 'x':
+			*swab = 1;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 2/7] commands: Get rid of mem_rw_buf
  2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 1/7] commands: Move mem_parse_options() to lib/misc.c Andrey Smirnov
@ 2019-01-23  1:13 ` Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 3/7] commands: Move /dev/mem driver to drivers/misc Andrey Smirnov
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

There doesn't seem to be any good reason for all of the memory
commands (md, mw, etc.) to rely on a shared pre-allocated buffer
anymore. So, to simplify things, drop the shared buffer and adjust all
of the utilites to allocate needed memory.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 commands/md.c     | 12 +++++++-----
 commands/mem.c    |  6 ------
 commands/memcmp.c | 16 ++++++++--------
 commands/memcpy.c | 10 ++++++----
 commands/memset.c |  2 --
 common/ratp/md.c  |  9 +++++----
 6 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/commands/md.c b/commands/md.c
index a495fc8b4..2389c12d1 100644
--- a/commands/md.c
+++ b/commands/md.c
@@ -34,8 +34,6 @@
 #include <linux/stat.h>
 #include <xfuncs.h>
 
-extern char *mem_rw_buf;
-
 static int do_mem_md(int argc, char *argv[])
 {
 	loff_t	start = 0, size = 0x100;
@@ -46,6 +44,7 @@ static int do_mem_md(int argc, char *argv[])
 	int mode = O_RWSIZE_4;
 	int swab = 0;
 	void *map;
+	void *buf = NULL;
 
 	if (argc < 2)
 		return COMMAND_ERROR_USAGE;
@@ -74,9 +73,11 @@ static int do_mem_md(int argc, char *argv[])
 		goto out;
 	}
 
+	buf = xmalloc(RW_BUF_SIZE);
+
 	do {
 		now = min(size, (loff_t)RW_BUF_SIZE);
-		r = read(fd, mem_rw_buf, now);
+		r = read(fd, buf, now);
 		if (r < 0) {
 			perror("read");
 			goto out;
@@ -84,8 +85,8 @@ static int do_mem_md(int argc, char *argv[])
 		if (!r)
 			goto out;
 
-		if ((ret = memory_display(mem_rw_buf, start, r,
-				mode >> O_RWSIZE_SHIFT, swab)))
+		if ((ret = memory_display(buf, start, r,
+					  mode >> O_RWSIZE_SHIFT, swab)))
 			goto out;
 
 		start += r;
@@ -93,6 +94,7 @@ static int do_mem_md(int argc, char *argv[])
 	} while (size);
 
 out:
+	free(buf);
 	close(fd);
 
 	return ret ? 1 : 0;
diff --git a/commands/mem.c b/commands/mem.c
index 62488bf52..8a47e1fe1 100644
--- a/commands/mem.c
+++ b/commands/mem.c
@@ -39,8 +39,6 @@
 #define PRINTF(fmt,args...)
 #endif
 
-char *mem_rw_buf;
-
 static struct cdev_operations memops = {
 	.read  = mem_read,
 	.write = mem_write,
@@ -73,10 +71,6 @@ static struct driver_d mem_drv = {
 
 static int mem_init(void)
 {
-	mem_rw_buf = malloc(RW_BUF_SIZE);
-	if(!mem_rw_buf)
-		return -ENOMEM;
-
 	add_mem_device("mem", 0, ~0, IORESOURCE_MEM_WRITEABLE);
 	return platform_driver_register(&mem_drv);
 }
diff --git a/commands/memcmp.c b/commands/memcmp.c
index 981c8cb38..48957b450 100644
--- a/commands/memcmp.c
+++ b/commands/memcmp.c
@@ -34,8 +34,6 @@
 #include <linux/stat.h>
 #include <xfuncs.h>
 
-extern char *mem_rw_buf;
-
 static char *devmem = "/dev/mem";
 
 static int do_memcmp(int argc, char *argv[])
@@ -45,7 +43,7 @@ static int do_memcmp(int argc, char *argv[])
 	char   *sourcefile = devmem;
 	char   *destfile = devmem;
 	int     sourcefd, destfd;
-	char   *rw_buf1;
+	char   *buf, *source_data, *dest_data;
 	int     ret = 1;
 	int     offset = 0;
 	struct  stat statbuf;
@@ -84,20 +82,22 @@ static int do_memcmp(int argc, char *argv[])
 		return 1;
 	}
 
-	rw_buf1 = xmalloc(RW_BUF_SIZE);
+	buf = xmalloc(RW_BUF_SIZE + RW_BUF_SIZE);
+	source_data = buf;
+	dest_data   = buf + RW_BUF_SIZE;
 
 	while (count > 0) {
 		int now, r1, r2, i;
 
 		now = min((loff_t)RW_BUF_SIZE, count);
 
-		r1 = read_full(sourcefd, mem_rw_buf, now);
+		r1 = read_full(sourcefd, source_data, now);
 		if (r1 < 0) {
 			perror("read");
 			goto out;
 		}
 
-		r2 = read_full(destfd, rw_buf1, now);
+		r2 = read_full(destfd, dest_data, now);
 		if (r2 < 0) {
 			perror("read");
 			goto out;
@@ -109,7 +109,7 @@ static int do_memcmp(int argc, char *argv[])
 		}
 
 		for (i = 0; i < now; i++) {
-			if (mem_rw_buf[i] != rw_buf1[i]) {
+			if (source_data[i] != dest_data[i]) {
 				printf("files differ at offset %d\n", offset);
 				goto out;
 			}
@@ -124,7 +124,7 @@ static int do_memcmp(int argc, char *argv[])
 out:
 	close(sourcefd);
 	close(destfd);
-	free(rw_buf1);
+	free(buf);
 
 	return ret;
 }
diff --git a/commands/memcpy.c b/commands/memcpy.c
index 168ef3b4f..ddaf767ea 100644
--- a/commands/memcpy.c
+++ b/commands/memcpy.c
@@ -34,8 +34,6 @@
 #include <linux/stat.h>
 #include <xfuncs.h>
 
-extern char *mem_rw_buf;
-
 static char *devmem = "/dev/mem";
 
 static int do_memcpy(int argc, char *argv[])
@@ -47,6 +45,7 @@ static int do_memcpy(int argc, char *argv[])
 	int mode = 0;
 	struct stat statbuf;
 	int ret = 0;
+	char *buf;
 
 	if (mem_parse_options(argc, argv, "bwlqs:d:", &mode, &sourcefile,
 			&destfile, NULL) < 0)
@@ -82,12 +81,14 @@ static int do_memcpy(int argc, char *argv[])
 		return 1;
 	}
 
+	buf = xmalloc(RW_BUF_SIZE);
+
 	while (count > 0) {
 		int now, r, w, tmp;
 
 		now = min((loff_t)RW_BUF_SIZE, count);
 
-		r = read(sourcefd, mem_rw_buf, now);
+		r = read(sourcefd, buf, now);
 		if (r < 0) {
 			perror("read");
 			goto out;
@@ -99,7 +100,7 @@ static int do_memcpy(int argc, char *argv[])
 		tmp = 0;
 		now = r;
 		while (now) {
-			w = write(destfd, mem_rw_buf + tmp, now);
+			w = write(destfd, buf + tmp, now);
 			if (w < 0) {
 				perror("write");
 				goto out;
@@ -123,6 +124,7 @@ static int do_memcpy(int argc, char *argv[])
 	}
 
 out:
+	free(buf);
 	close(sourcefd);
 	close(destfd);
 
diff --git a/commands/memset.c b/commands/memset.c
index f99bf86c0..b0770159f 100644
--- a/commands/memset.c
+++ b/commands/memset.c
@@ -34,8 +34,6 @@
 #include <linux/stat.h>
 #include <xfuncs.h>
 
-extern char *mem_rw_buf;
-
 static int do_memset(int argc, char *argv[])
 {
 	loff_t	s, c, n;
diff --git a/common/ratp/md.c b/common/ratp/md.c
index ce343d7c7..a25cbf112 100644
--- a/common/ratp/md.c
+++ b/common/ratp/md.c
@@ -59,8 +59,6 @@ struct ratp_bb_md_response {
 	uint8_t  buffer[];
 } __packed;
 
-extern char *mem_rw_buf;
-
 static int do_ratp_mem_md(const char *filename,
 			  loff_t start,
 			  loff_t size,
@@ -70,6 +68,7 @@ static int do_ratp_mem_md(const char *filename,
 	int ret = 0;
 	int fd;
 	void *map;
+	char *buf = NULL;
 
 	fd = open_and_lseek(filename, O_RWSIZE_1 | O_RDONLY, start);
 	if (fd < 0)
@@ -81,10 +80,11 @@ static int do_ratp_mem_md(const char *filename,
 		goto out;
 	}
 
+	buf = xmalloc(RW_BUF_SIZE);
 	t = 0;
 	do {
 		now = min(size, (loff_t)RW_BUF_SIZE);
-		r = read(fd, mem_rw_buf, now);
+		r = read(fd, buf, now);
 		if (r < 0) {
 			ret = -errno;
 			perror("read");
@@ -93,13 +93,14 @@ static int do_ratp_mem_md(const char *filename,
 		if (!r)
 			goto out;
 
-		memcpy(output + t, (uint8_t *)(mem_rw_buf), r);
+		memcpy(output + t, buf, r);
 
 		size  -= r;
 		t     += r;
 	} while (size);
 
 out:
+	free(buf);
 	close(fd);
 
 	return ret;
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 3/7] commands: Move /dev/mem driver to drivers/misc
  2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 1/7] commands: Move mem_parse_options() to lib/misc.c Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 2/7] commands: Get rid of mem_rw_buf Andrey Smirnov
@ 2019-01-23  1:13 ` Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 4/7] fs: Change error checking logic for fsdrv->lseek() call Andrey Smirnov
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

With all other code gone from commands/mem.c, move it into
driver/misc, where it fits better. While at it, expose it directly via
a Kconfig options instead of relying on CONFIG_COMPILE_MEMORY

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 commands/Kconfig      | 17 ++++------
 commands/Makefile     |  1 -
 commands/mem.c        | 77 -------------------------------------------
 drivers/misc/Kconfig  |  3 ++
 drivers/misc/Makefile |  1 +
 drivers/misc/mem.c    | 45 +++++++++++++++++++++++++
 6 files changed, 55 insertions(+), 89 deletions(-)
 delete mode 100644 commands/mem.c
 create mode 100644 drivers/misc/mem.c

diff --git a/commands/Kconfig b/commands/Kconfig
index 45592f26c..901036f59 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -14,11 +14,6 @@ config COMPILE_HASH
 	help
 	  Turns on compilation of digest.c
 
-config COMPILE_MEMORY
-	bool
-	help
-	  Turns on compilation of mem.c
-
 menu "Commands"
 
 
@@ -1493,7 +1488,7 @@ config CMD_CRC_CMP
 config CMD_MD
 	tristate
 	default y
-	select COMPILE_MEMORY
+	select DEV_MEM
 	prompt "md"
 	help
 	  Memory display
@@ -1517,7 +1512,7 @@ config CMD_MD
 config CMD_MEMCMP
 	tristate
 	default y
-	select COMPILE_MEMORY
+	select DEV_MEM
 	prompt "memcmp"
 	help
 	  Memory compare
@@ -1539,7 +1534,7 @@ config CMD_MEMCMP
 config CMD_MEMCPY
 	tristate
 	default y
-	select COMPILE_MEMORY
+	select DEV_MEM
 	prompt "memcpy"
 	help
 	  Memory copy
@@ -1558,7 +1553,7 @@ config CMD_MEMCPY
 config CMD_MEMSET
 	tristate
 	default y
-	select COMPILE_MEMORY
+	select DEV_MEM
 	prompt "memset"
 	help
 	  Memory fill
@@ -1591,7 +1586,7 @@ config CMD_MEMTEST
 
 config CMD_MM
 	tristate
-	select COMPILE_MEMORY
+	select DEV_MEM
 	prompt "memory modify (mm)"
 	help
 	  Memory modify with mask
@@ -1609,7 +1604,7 @@ config CMD_MM
 config CMD_MW
 	tristate
 	default y
-	select COMPILE_MEMORY
+	select DEV_MEM
 	prompt "mw"
 	help
 	  Memory write
diff --git a/commands/Makefile b/commands/Makefile
index ecd2c99e1..ce4c4f6eb 100644
--- a/commands/Makefile
+++ b/commands/Makefile
@@ -1,7 +1,6 @@
 obj-$(CONFIG_STDDEV)		+= stddev.o
 obj-$(CONFIG_CMD_DIGEST)	+= digest.o
 obj-$(CONFIG_COMPILE_HASH)	+= hashsum.o
-obj-$(CONFIG_COMPILE_MEMORY)	+= mem.o
 obj-$(CONFIG_CMD_BOOTM)		+= bootm.o
 obj-$(CONFIG_CMD_UIMAGE)	+= uimage.o
 obj-$(CONFIG_CMD_LINUX16)	+= linux16.o
diff --git a/commands/mem.c b/commands/mem.c
deleted file mode 100644
index 8a47e1fe1..000000000
--- a/commands/mem.c
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (c) 2011 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
- *
- * See file CREDITS for list of people who contributed to this
- * project.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-/*
- * Memory Functions
- *
- * Copied from FADS ROM, Dan Malek (dmalek@jlc.net)
- */
-
-#include <common.h>
-#include <command.h>
-#include <init.h>
-#include <driver.h>
-#include <malloc.h>
-#include <errno.h>
-#include <fs.h>
-#include <fcntl.h>
-#include <getopt.h>
-#include <linux/stat.h>
-#include <xfuncs.h>
-
-#ifdef	CMD_MEM_DEBUG
-#define	PRINTF(fmt,args...)	printf (fmt ,##args)
-#else
-#define PRINTF(fmt,args...)
-#endif
-
-static struct cdev_operations memops = {
-	.read  = mem_read,
-	.write = mem_write,
-	.memmap = generic_memmap_rw,
-	.lseek = dev_lseek_default,
-};
-
-static int mem_probe(struct device_d *dev)
-{
-	struct cdev *cdev;
-
-	cdev = xzalloc(sizeof (*cdev));
-	dev->priv = cdev;
-
-	cdev->name = (char*)dev->resource[0].name;
-	cdev->size = min_t(unsigned long long, resource_size(&dev->resource[0]),
-			   S64_MAX);
-	cdev->ops = &memops;
-	cdev->dev = dev;
-
-	devfs_create(cdev);
-
-	return 0;
-}
-
-static struct driver_d mem_drv = {
-	.name  = "mem",
-	.probe = mem_probe,
-};
-
-static int mem_init(void)
-{
-	add_mem_device("mem", 0, ~0, IORESOURCE_MEM_WRITEABLE);
-	return platform_driver_register(&mem_drv);
-}
-device_initcall(mem_init);
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index df74cca97..f547024bd 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -24,4 +24,7 @@ config BLINK_ENCODER
         tristate "Blinking LED encoder"
 	depends on LED && LED_TRIGGERS
 
+config DEV_MEM
+        bool "Generic memory I/O device (/dev/mem)"
+
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 2e80f1a63..8b5140536 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_JTAG)		+= jtag.o
 obj-$(CONFIG_SRAM)		+= sram.o
 obj-$(CONFIG_STATE_DRV)		+= state.o
 obj-$(CONFIG_BLINK_ENCODER)	+= blink-encoder.o
+obj-$(CONFIG_DEV_MEM)		+= mem.o
diff --git a/drivers/misc/mem.c b/drivers/misc/mem.c
new file mode 100644
index 000000000..d829af724
--- /dev/null
+++ b/drivers/misc/mem.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2011 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
+ */
+
+#include <common.h>
+#include <driver.h>
+#include <init.h>
+
+static struct cdev_operations memops = {
+	.read  = mem_read,
+	.write = mem_write,
+	.memmap = generic_memmap_rw,
+	.lseek = dev_lseek_default,
+};
+
+static int mem_probe(struct device_d *dev)
+{
+	struct cdev *cdev;
+
+	cdev = xzalloc(sizeof (*cdev));
+	dev->priv = cdev;
+
+	cdev->name = (char*)dev->resource[0].name;
+	cdev->size = min_t(unsigned long long, resource_size(&dev->resource[0]),
+			   S64_MAX);
+	cdev->ops = &memops;
+	cdev->dev = dev;
+
+	devfs_create(cdev);
+
+	return 0;
+}
+
+static struct driver_d mem_drv = {
+	.name  = "mem",
+	.probe = mem_probe,
+};
+
+static int mem_init(void)
+{
+	add_mem_device("mem", 0, ~0, IORESOURCE_MEM_WRITEABLE);
+	return platform_driver_register(&mem_drv);
+}
+device_initcall(mem_init);
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 4/7] fs: Change error checking logic for fsdrv->lseek() call
  2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
                   ` (2 preceding siblings ...)
  2019-01-23  1:13 ` [PATCH 3/7] commands: Move /dev/mem driver to drivers/misc Andrey Smirnov
@ 2019-01-23  1:13 ` Andrey Smirnov
  2019-01-24  7:44   ` Sascha Hauer
  2019-01-23  1:13 ` [PATCH 5/7] fs: Calculate new position before validtiy check in lseek() Andrey Smirnov
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

On 32-bit systems, cheking for IS_ERR_VALUE(pos) is not
correct. Expanding that code we get (loff_t cast is added for clarity):

 (loff_t)pos >= (unsigned long)-MAX_ERRNO

given that loff_t is a 64-bit signed value, any perfectly valid seek
offset that is greater than 0xffffc000 will result in false
positive. Change the logic to check if position returned by
fsdrv->lseek() is what's been requested. If it is, we can assume that
operation was succesfull. If not, that's likely means failure and
return value is a negative error code.

This should accomodate both 32-bit systems, where we /dev/mem doesn't
present any range problems, as well as 64-bit systems where both file
offset and size of /dev/mem couldn't really be correctly captured by
loff_t and we have to rely on 2's complement and overflow.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 fs/fs.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index a304bf186..6a62fb98b 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -405,8 +405,7 @@ loff_t lseek(int fildes, loff_t offset, int whence)
 {
 	struct fs_driver_d *fsdrv;
 	FILE *f;
-	loff_t pos;
-	int ret;
+	loff_t pos, ret;
 
 	if (check_fd(fildes))
 		return -1;
@@ -442,13 +441,11 @@ loff_t lseek(int fildes, loff_t offset, int whence)
 		goto out;
 	}
 
-	pos = fsdrv->lseek(&f->fsdev->dev, f, pos);
-	if (IS_ERR_VALUE(pos)) {
-		errno = -pos;
-		return -1;
-	}
+	ret = fsdrv->lseek(&f->fsdev->dev, f, pos);
+	if (ret != pos)
+		goto out;
 
-	return pos;
+	return ret;
 
 out:
 	if (ret)
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 5/7] fs: Calculate new position before validtiy check in lseek()
  2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
                   ` (3 preceding siblings ...)
  2019-01-23  1:13 ` [PATCH 4/7] fs: Change error checking logic for fsdrv->lseek() call Andrey Smirnov
@ 2019-01-23  1:13 ` Andrey Smirnov
  2019-01-24  7:52   ` Sascha Hauer
  2019-01-23  1:13 ` [PATCH 6/7] fs: Add support for files larger than MAX_LFS_FILESIZE Andrey Smirnov
  2019-01-23  1:13 ` [PATCH 7/7] misc: mem: Set correct size for /dev/mem Andrey Smirnov
  6 siblings, 1 reply; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

Calculate new position before validtiy check in lseek() to simplify
code a bit as well as make following commit simpler. This should be
harmless thing to do, since we don't actually use calculated value
unless it passes the validity check.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 fs/fs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index 6a62fb98b..9372b9981 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -421,21 +421,21 @@ loff_t lseek(int fildes, loff_t offset, int whence)
 
 	switch (whence) {
 	case SEEK_SET:
+		pos = offset;
 		if (f->size != FILE_SIZE_STREAM && offset > f->size)
 			goto out;
 		if (IS_ERR_VALUE(offset))
 			goto out;
-		pos = offset;
 		break;
 	case SEEK_CUR:
-		if (f->size != FILE_SIZE_STREAM && offset + f->pos > f->size)
-			goto out;
 		pos = f->pos + offset;
+		if (f->size != FILE_SIZE_STREAM && pos > f->size)
+			goto out;
 		break;
 	case SEEK_END:
+		pos = f->size + offset;
 		if (offset > 0)
 			goto out;
-		pos = f->size + offset;
 		break;
 	default:
 		goto out;
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 6/7] fs: Add support for files larger than MAX_LFS_FILESIZE
  2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
                   ` (4 preceding siblings ...)
  2019-01-23  1:13 ` [PATCH 5/7] fs: Calculate new position before validtiy check in lseek() Andrey Smirnov
@ 2019-01-23  1:13 ` Andrey Smirnov
  2019-01-24  8:48   ` Sascha Hauer
  2019-01-23  1:13 ` [PATCH 7/7] misc: mem: Set correct size for /dev/mem Andrey Smirnov
  6 siblings, 1 reply; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

On 64-bit platforms /dev/mem exceeds the size supported by loff_t and
needs special treatment within the rest of FS API. Specifically
lseek() needs to be modified to make sure it does the right thing.

Prievious attempt at fixing this issue by using IS_ERR_VALUE()

e10efc5080 ("fs: fix memory access via /dev/mem for MIPS64")

doesn't really work 100% on 64-bit platforms, becuase it still leaves
out a number of perfectly valid offsets (e.g. "md 0xffffffffffffff00"
doesn't work) . Moreso it breaks lseek() on 32-bit platforms, since
IS_ERR_VALUE will retrurn true for any offset that is >= (unsigned
long) -MAX_ERRNO.

In order to fix this issue on both 32 and 64 bit platforms, introduce
DEVFS_UNBOUNDED flag that cdevs can use to denote that they span all
64-bit address space and effectively have not limits. To propagate
that info to FS layer, add "unbounded" boolean to FILE. As a last step
modify lseek() to be aware of that field and do the right checks in
that case.

Note, that since loff_t has no problem covering all of address space
on 32-bit platforms, DEVFS_UNBOUNDED is defined to expand into 0 and
not be settable there.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/misc/mem.c |  1 +
 fs/devfs.c         |  2 ++
 fs/fs.c            | 29 +++++++++++++++++++++++++----
 include/driver.h   |  1 +
 include/fs.h       |  1 +
 5 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/mem.c b/drivers/misc/mem.c
index d829af724..b27865b9e 100644
--- a/drivers/misc/mem.c
+++ b/drivers/misc/mem.c
@@ -21,6 +21,7 @@ static int mem_probe(struct device_d *dev)
 	cdev = xzalloc(sizeof (*cdev));
 	dev->priv = cdev;
 
+	cdev->flags = DEVFS_UNBOUNDED;
 	cdev->name = (char*)dev->resource[0].name;
 	cdev->size = min_t(unsigned long long, resource_size(&dev->resource[0]),
 			   S64_MAX);
diff --git a/fs/devfs.c b/fs/devfs.c
index 81ae2c25a..68a5b7f23 100644
--- a/fs/devfs.c
+++ b/fs/devfs.c
@@ -120,6 +120,8 @@ static int devfs_open(struct device_d *_dev, FILE *f, const char *filename)
 	struct cdev *cdev = node->cdev;
 	int ret;
 
+
+	f->unbounded = cdev->flags & DEVFS_UNBOUNDED;
 	f->size = cdev->flags & DEVFS_IS_CHARACTER_DEV ?
 			FILE_SIZE_STREAM : cdev->size;
 	f->priv = cdev;
diff --git a/fs/fs.c b/fs/fs.c
index 9372b9981..48ca382df 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -422,20 +422,41 @@ loff_t lseek(int fildes, loff_t offset, int whence)
 	switch (whence) {
 	case SEEK_SET:
 		pos = offset;
+		if (unlikely(f->unbounded))
+			break;
 		if (f->size != FILE_SIZE_STREAM && offset > f->size)
 			goto out;
-		if (IS_ERR_VALUE(offset))
+		if (offset < 0)
 			goto out;
 		break;
 	case SEEK_CUR:
 		pos = f->pos + offset;
+		if (unlikely(f->unbounded)) {
+			/*
+			 * Check that we won't wrap around when
+			 * seeking
+			 */
+			u64 p, o;
+
+			p = f->pos;
+			o = offset;
+
+			if (p > U64_MAX - o)
+				goto out;
+			break;
+		}
+
 		if (f->size != FILE_SIZE_STREAM && pos > f->size)
 			goto out;
 		break;
 	case SEEK_END:
-		pos = f->size + offset;
-		if (offset > 0)
-			goto out;
+		if (unlikely(f->unbounded)) {
+			pos = U64_MAX - (u64)offset;
+		} else {
+			pos = f->size + offset;
+			if (offset > 0)
+				goto out;
+		}
 		break;
 	default:
 		goto out;
diff --git a/include/driver.h b/include/driver.h
index 3d9970df5..39bce0414 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -501,6 +501,7 @@ int cdev_erase(struct cdev *cdev, loff_t count, loff_t offset);
 #define DEVFS_PARTITION_READONLY	(1U << 1)
 #define DEVFS_IS_CHARACTER_DEV		(1 << 3)
 #define DEVFS_PARTITION_FROM_TABLE	(1 << 4)
+#define DEVFS_UNBOUNDED			(IS_ENABLED(CONFIG_CPU_64) << 5)
 
 struct cdev *devfs_add_partition(const char *devname, loff_t offset,
 		loff_t size, unsigned int flags, const char *name);
diff --git a/include/fs.h b/include/fs.h
index f1514afa9..5d1510838 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -31,6 +31,7 @@ typedef struct filep {
 	/* private fields. Mapping between FILE and filedescriptor number     */
 	int no;
 	char in_use;
+	bool unbounded;
 
 	struct inode *f_inode;
 	struct dentry *dentry;
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 7/7] misc: mem: Set correct size for /dev/mem
  2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
                   ` (5 preceding siblings ...)
  2019-01-23  1:13 ` [PATCH 6/7] fs: Add support for files larger than MAX_LFS_FILESIZE Andrey Smirnov
@ 2019-01-23  1:13 ` Andrey Smirnov
  6 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-23  1:13 UTC (permalink / raw)
  To: barebox; +Cc: Andrey Smirnov

On 64-bit platfoms /dev/mem should have the size of 2^64 bytes, not
2^63 which would result from using S64_MAX. This also has a side
effect of setting "size" in FILE to FILE_SIZE_STREAM, disabling a
number of codepaths that are not applicable to /dev/mem anyway (see
__read() and __write()).

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/misc/mem.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/misc/mem.c b/drivers/misc/mem.c
index b27865b9e..89ec920b2 100644
--- a/drivers/misc/mem.c
+++ b/drivers/misc/mem.c
@@ -23,8 +23,7 @@ static int mem_probe(struct device_d *dev)
 
 	cdev->flags = DEVFS_UNBOUNDED;
 	cdev->name = (char*)dev->resource[0].name;
-	cdev->size = min_t(unsigned long long, resource_size(&dev->resource[0]),
-			   S64_MAX);
+	cdev->size = resource_size(&dev->resource[0]);
 	cdev->ops = &memops;
 	cdev->dev = dev;
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/7] fs: Change error checking logic for fsdrv->lseek() call
  2019-01-23  1:13 ` [PATCH 4/7] fs: Change error checking logic for fsdrv->lseek() call Andrey Smirnov
@ 2019-01-24  7:44   ` Sascha Hauer
  2019-01-24 19:19     ` Andrey Smirnov
  0 siblings, 1 reply; 14+ messages in thread
From: Sascha Hauer @ 2019-01-24  7:44 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: barebox

On Tue, Jan 22, 2019 at 05:13:35PM -0800, Andrey Smirnov wrote:
> On 32-bit systems, cheking for IS_ERR_VALUE(pos) is not
> correct. Expanding that code we get (loff_t cast is added for clarity):
> 
>  (loff_t)pos >= (unsigned long)-MAX_ERRNO
> 
> given that loff_t is a 64-bit signed value, any perfectly valid seek
> offset that is greater than 0xffffc000 will result in false
> positive. Change the logic to check if position returned by
> fsdrv->lseek() is what's been requested. If it is, we can assume that
> operation was succesfull. If not, that's likely means failure and
> return value is a negative error code.
> 
> This should accomodate both 32-bit systems, where we /dev/mem doesn't
> present any range problems, as well as 64-bit systems where both file
> offset and size of /dev/mem couldn't really be correctly captured by
> loff_t and we have to rely on 2's complement and overflow.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  fs/fs.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/fs.c b/fs/fs.c
> index a304bf186..6a62fb98b 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -405,8 +405,7 @@ loff_t lseek(int fildes, loff_t offset, int whence)
>  {
>  	struct fs_driver_d *fsdrv;
>  	FILE *f;
> -	loff_t pos;
> -	int ret;
> +	loff_t pos, ret;
>  
>  	if (check_fd(fildes))
>  		return -1;
> @@ -442,13 +441,11 @@ loff_t lseek(int fildes, loff_t offset, int whence)
>  		goto out;
>  	}
>  
> -	pos = fsdrv->lseek(&f->fsdev->dev, f, pos);
> -	if (IS_ERR_VALUE(pos)) {
> -		errno = -pos;
> -		return -1;
> -	}
> +	ret = fsdrv->lseek(&f->fsdev->dev, f, pos);
> +	if (ret != pos)
> +		goto out;

There's no point in returning the current position from fsdrv->lseek
when the desired position is already an input parameter. I think we
should change the prototype of fsdrv->lseek to just return an error code.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 5/7] fs: Calculate new position before validtiy check in lseek()
  2019-01-23  1:13 ` [PATCH 5/7] fs: Calculate new position before validtiy check in lseek() Andrey Smirnov
@ 2019-01-24  7:52   ` Sascha Hauer
  2019-01-24 19:37     ` Andrey Smirnov
  0 siblings, 1 reply; 14+ messages in thread
From: Sascha Hauer @ 2019-01-24  7:52 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: barebox

On Tue, Jan 22, 2019 at 05:13:36PM -0800, Andrey Smirnov wrote:
> Calculate new position before validtiy check in lseek() to simplify
> code a bit as well as make following commit simpler. This should be
> harmless thing to do, since we don't actually use calculated value
> unless it passes the validity check.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  fs/fs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fs.c b/fs/fs.c
> index 6a62fb98b..9372b9981 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -421,21 +421,21 @@ loff_t lseek(int fildes, loff_t offset, int whence)
>  
>  	switch (whence) {
>  	case SEEK_SET:
> +		pos = offset;
>  		if (f->size != FILE_SIZE_STREAM && offset > f->size)
>  			goto out;
>  		if (IS_ERR_VALUE(offset))
>  			goto out;

This test looks quite unnecessary. Can we remove it? git blame points to
me of course, but I can't make any sense of it.

> -		pos = offset;
>  		break;
>  	case SEEK_CUR:
> -		if (f->size != FILE_SIZE_STREAM && offset + f->pos > f->size)
> -			goto out;
>  		pos = f->pos + offset;
> +		if (f->size != FILE_SIZE_STREAM && pos > f->size)
> +			goto out;
>  		break;
>  	case SEEK_END:
> +		pos = f->size + offset;
>  		if (offset > 0)
>  			goto out;
> -		pos = f->size + offset;

When starting to shift the validity checks around, can't we just do them
after the switch/case instead of in each branch?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 6/7] fs: Add support for files larger than MAX_LFS_FILESIZE
  2019-01-23  1:13 ` [PATCH 6/7] fs: Add support for files larger than MAX_LFS_FILESIZE Andrey Smirnov
@ 2019-01-24  8:48   ` Sascha Hauer
  2019-01-24 19:43     ` Andrey Smirnov
  0 siblings, 1 reply; 14+ messages in thread
From: Sascha Hauer @ 2019-01-24  8:48 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: barebox

On Tue, Jan 22, 2019 at 05:13:37PM -0800, Andrey Smirnov wrote:
> On 64-bit platforms /dev/mem exceeds the size supported by loff_t and
> needs special treatment within the rest of FS API. Specifically
> lseek() needs to be modified to make sure it does the right thing.
> 
> Prievious attempt at fixing this issue by using IS_ERR_VALUE()
> 
> e10efc5080 ("fs: fix memory access via /dev/mem for MIPS64")
> 
> doesn't really work 100% on 64-bit platforms, becuase it still leaves
> out a number of perfectly valid offsets (e.g. "md 0xffffffffffffff00"
> doesn't work) . Moreso it breaks lseek() on 32-bit platforms, since
> IS_ERR_VALUE will retrurn true for any offset that is >= (unsigned
> long) -MAX_ERRNO.
> 
> In order to fix this issue on both 32 and 64 bit platforms, introduce
> DEVFS_UNBOUNDED flag that cdevs can use to denote that they span all
> 64-bit address space and effectively have not limits. To propagate
> that info to FS layer, add "unbounded" boolean to FILE. As a last step
> modify lseek() to be aware of that field and do the right checks in
> that case.
> 
> Note, that since loff_t has no problem covering all of address space
> on 32-bit platforms, DEVFS_UNBOUNDED is defined to expand into 0 and
> not be settable there.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> @@ -422,20 +422,41 @@ loff_t lseek(int fildes, loff_t offset, int whence)

lseek takes a signed loff_t argument. We store the file size in an also
signed variable. An lseek to the end of a file covering the whole 64bit
address space (0xffffffffffffffff) will always return an error as
(loff_t)-1 is both the position and the error code.

I think instead of casting loff_t to an unsigned type whenever we find
it convenient and then still not having enough bits for storing the
filesize of 0x10000000000000000 we should rather face the fact that our
maximum filesize is only half of the 64bit address space.

To put it differently we should think about creating a second /dev/mem
for the upper half of the 64bit address space.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/7] fs: Change error checking logic for fsdrv->lseek() call
  2019-01-24  7:44   ` Sascha Hauer
@ 2019-01-24 19:19     ` Andrey Smirnov
  0 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-24 19:19 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: Barebox List

On Wed, Jan 23, 2019 at 11:44 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Tue, Jan 22, 2019 at 05:13:35PM -0800, Andrey Smirnov wrote:
> > On 32-bit systems, cheking for IS_ERR_VALUE(pos) is not
> > correct. Expanding that code we get (loff_t cast is added for clarity):
> >
> >  (loff_t)pos >= (unsigned long)-MAX_ERRNO
> >
> > given that loff_t is a 64-bit signed value, any perfectly valid seek
> > offset that is greater than 0xffffc000 will result in false
> > positive. Change the logic to check if position returned by
> > fsdrv->lseek() is what's been requested. If it is, we can assume that
> > operation was succesfull. If not, that's likely means failure and
> > return value is a negative error code.
> >
> > This should accomodate both 32-bit systems, where we /dev/mem doesn't
> > present any range problems, as well as 64-bit systems where both file
> > offset and size of /dev/mem couldn't really be correctly captured by
> > loff_t and we have to rely on 2's complement and overflow.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  fs/fs.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/fs.c b/fs/fs.c
> > index a304bf186..6a62fb98b 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -405,8 +405,7 @@ loff_t lseek(int fildes, loff_t offset, int whence)
> >  {
> >       struct fs_driver_d *fsdrv;
> >       FILE *f;
> > -     loff_t pos;
> > -     int ret;
> > +     loff_t pos, ret;
> >
> >       if (check_fd(fildes))
> >               return -1;
> > @@ -442,13 +441,11 @@ loff_t lseek(int fildes, loff_t offset, int whence)
> >               goto out;
> >       }
> >
> > -     pos = fsdrv->lseek(&f->fsdev->dev, f, pos);
> > -     if (IS_ERR_VALUE(pos)) {
> > -             errno = -pos;
> > -             return -1;
> > -     }
> > +     ret = fsdrv->lseek(&f->fsdev->dev, f, pos);
> > +     if (ret != pos)
> > +             goto out;
>
> There's no point in returning the current position from fsdrv->lseek
> when the desired position is already an input parameter. I think we
> should change the prototype of fsdrv->lseek to just return an error code.
>

OK, sure, will do in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 5/7] fs: Calculate new position before validtiy check in lseek()
  2019-01-24  7:52   ` Sascha Hauer
@ 2019-01-24 19:37     ` Andrey Smirnov
  0 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-24 19:37 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: Barebox List

On Wed, Jan 23, 2019 at 11:52 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Tue, Jan 22, 2019 at 05:13:36PM -0800, Andrey Smirnov wrote:
> > Calculate new position before validtiy check in lseek() to simplify
> > code a bit as well as make following commit simpler. This should be
> > harmless thing to do, since we don't actually use calculated value
> > unless it passes the validity check.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  fs/fs.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/fs.c b/fs/fs.c
> > index 6a62fb98b..9372b9981 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -421,21 +421,21 @@ loff_t lseek(int fildes, loff_t offset, int whence)
> >
> >       switch (whence) {
> >       case SEEK_SET:
> > +             pos = offset;
> >               if (f->size != FILE_SIZE_STREAM && offset > f->size)
> >                       goto out;
> >               if (IS_ERR_VALUE(offset))
> >                       goto out;
>
> This test looks quite unnecessary. Can we remove it? git blame points to
> me of course, but I can't make any sense of it.
>

It originally was "if (offset < 0)", which I think is
reasonable/necessary given how we probably don't want to allow
specifying negative "offset" with SEEK_SET, but allow it for SEEK_END
and SEEK_CUR. Then in attempt to fix /dev/mem accesses on 64-bit MIPS
original check was changed to IS_ERR_VALUE(offset), which made the
original problem less visible (by narrowing down offsets that are
forbidden) which broke lseek() on 32-bit platforms. Hope this
clarifies things a bit.

> > -             pos = offset;
> >               break;
> >       case SEEK_CUR:
> > -             if (f->size != FILE_SIZE_STREAM && offset + f->pos > f->size)
> > -                     goto out;
> >               pos = f->pos + offset;
> > +             if (f->size != FILE_SIZE_STREAM && pos > f->size)
> > +                     goto out;
> >               break;
> >       case SEEK_END:
> > +             pos = f->size + offset;
> >               if (offset > 0)
> >                       goto out;
> > -             pos = f->size + offset;
>
> When starting to shift the validity checks around, can't we just do them
> after the switch/case instead of in each branch?
>

I think we can move "if (f->size != FILE_SIZE_STREAM && pos >
f->size)" outside. Not sure about some "whence" specific checks. I'll
give it a try in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 6/7] fs: Add support for files larger than MAX_LFS_FILESIZE
  2019-01-24  8:48   ` Sascha Hauer
@ 2019-01-24 19:43     ` Andrey Smirnov
  0 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-01-24 19:43 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: Barebox List

On Thu, Jan 24, 2019 at 12:48 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Tue, Jan 22, 2019 at 05:13:37PM -0800, Andrey Smirnov wrote:
> > On 64-bit platforms /dev/mem exceeds the size supported by loff_t and
> > needs special treatment within the rest of FS API. Specifically
> > lseek() needs to be modified to make sure it does the right thing.
> >
> > Prievious attempt at fixing this issue by using IS_ERR_VALUE()
> >
> > e10efc5080 ("fs: fix memory access via /dev/mem for MIPS64")
> >
> > doesn't really work 100% on 64-bit platforms, becuase it still leaves
> > out a number of perfectly valid offsets (e.g. "md 0xffffffffffffff00"
> > doesn't work) . Moreso it breaks lseek() on 32-bit platforms, since
> > IS_ERR_VALUE will retrurn true for any offset that is >= (unsigned
> > long) -MAX_ERRNO.
> >
> > In order to fix this issue on both 32 and 64 bit platforms, introduce
> > DEVFS_UNBOUNDED flag that cdevs can use to denote that they span all
> > 64-bit address space and effectively have not limits. To propagate
> > that info to FS layer, add "unbounded" boolean to FILE. As a last step
> > modify lseek() to be aware of that field and do the right checks in
> > that case.
> >
> > Note, that since loff_t has no problem covering all of address space
> > on 32-bit platforms, DEVFS_UNBOUNDED is defined to expand into 0 and
> > not be settable there.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > @@ -422,20 +422,41 @@ loff_t lseek(int fildes, loff_t offset, int whence)
>
> lseek takes a signed loff_t argument. We store the file size in an also
> signed variable. An lseek to the end of a file covering the whole 64bit
> address space (0xffffffffffffffff) will always return an error as
> (loff_t)-1 is both the position and the error code.
>
> I think instead of casting loff_t to an unsigned type whenever we find
> it convenient and then still not having enough bits for storing the
> filesize of 0x10000000000000000 we should rather face the fact that our
> maximum filesize is only half of the 64bit address space.
>
> To put it differently we should think about creating a second /dev/mem
> for the upper half of the 64bit address space.
>

OK, I was trying to preserve as much backwards compatibility as
possible in v1. If creating a separate device is on the table, I'd be
more that happy to make that change in v2, since it'll make a number
of codepaths simpler and saner.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-01-24 19:43 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-23  1:13 [PATCH 0/7] 32-bit lseek and /dev/mem fixes/improvements Andrey Smirnov
2019-01-23  1:13 ` [PATCH 1/7] commands: Move mem_parse_options() to lib/misc.c Andrey Smirnov
2019-01-23  1:13 ` [PATCH 2/7] commands: Get rid of mem_rw_buf Andrey Smirnov
2019-01-23  1:13 ` [PATCH 3/7] commands: Move /dev/mem driver to drivers/misc Andrey Smirnov
2019-01-23  1:13 ` [PATCH 4/7] fs: Change error checking logic for fsdrv->lseek() call Andrey Smirnov
2019-01-24  7:44   ` Sascha Hauer
2019-01-24 19:19     ` Andrey Smirnov
2019-01-23  1:13 ` [PATCH 5/7] fs: Calculate new position before validtiy check in lseek() Andrey Smirnov
2019-01-24  7:52   ` Sascha Hauer
2019-01-24 19:37     ` Andrey Smirnov
2019-01-23  1:13 ` [PATCH 6/7] fs: Add support for files larger than MAX_LFS_FILESIZE Andrey Smirnov
2019-01-24  8:48   ` Sascha Hauer
2019-01-24 19:43     ` Andrey Smirnov
2019-01-23  1:13 ` [PATCH 7/7] misc: mem: Set correct size for /dev/mem Andrey Smirnov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox