[PATCH 0/6] Support for digesting large files (> 4 GiB)

[PATCH 0/6] Support for digesting large files (> 4 GiB)

[Andrey Smirnov]

Everyone:

This is the series of patches I made while to fix incorrect behaviour
while trying to md5sum a 128 GiB file. Most of the patches should be
pretty straightforward. If "crypto: digest: Split memory vs. file code
into separate functions" is too controversial it can be dropped.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (6):
  treewide: Introduce MAP_FAILED and replace ad-hoc constants with it
  crypto: digest: Remove unused variable
  crypto: digest: Replace 4096 with PAGE_SIZE
  crypto: digest: Split memory vs. file code into separate functions
  commands: digest: Use MAX_LFS_FILESIZE instead of ~0
  crypto: digest: Change the signature of digest_file_window()

 commands/digest.c |   3 +-
 commands/go.c     |   2 +-
 commands/md.c     |   2 +-
 common/ratp/md.c  |   2 +-
 crypto/digest.c   | 122 +++++++++++++++++++++++++++-------------------
 fs/fs.c           |   2 +-
 include/digest.h  |   2 +-
 include/fs.h      |   2 +
 8 files changed, 80 insertions(+), 57 deletions(-)

-- 
2.20.1


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

[PATCH 1/6] treewide: Introduce MAP_FAILED and replace ad-hoc constants with it

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 commands/go.c    | 2 +-
 commands/md.c    | 2 +-
 common/ratp/md.c | 2 +-
 crypto/digest.c  | 2 +-
 fs/fs.c          | 2 +-
 include/fs.h     | 2 ++
 6 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/commands/go.c b/commands/go.c
index fb319b320c..ecc2ceb6e4 100644
--- a/commands/go.c
+++ b/commands/go.c
@@ -45,7 +45,7 @@ static int do_go(int argc, char *argv[])
 		}
 
 		addr = memmap(fd, PROT_READ);
-		if (addr == (void *)-1) {
+		if (addr == MAP_FAILED) {
 			perror("memmap");
 			goto out;
 		}
diff --git a/commands/md.c b/commands/md.c
index 3e83c723a3..a495fc8b41 100644
--- a/commands/md.c
+++ b/commands/md.c
@@ -68,7 +68,7 @@ static int do_mem_md(int argc, char *argv[])
 		return 1;
 
 	map = memmap(fd, PROT_READ);
-	if (map != (void *)-1) {
+	if (map != MAP_FAILED) {
 		ret = memory_display(map + start, start, size,
 				mode >> O_RWSIZE_SHIFT, swab);
 		goto out;
diff --git a/common/ratp/md.c b/common/ratp/md.c
index 9ce7e99dfd..ce343d7c7b 100644
--- a/common/ratp/md.c
+++ b/common/ratp/md.c
@@ -76,7 +76,7 @@ static int do_ratp_mem_md(const char *filename,
 		return -errno;
 
 	map = memmap(fd, PROT_READ);
-	if (map != (void *)-1) {
+	if (map != MAP_FAILED) {
 		memcpy(output, (uint8_t *)(map + start), size);
 		goto out;
 	}
diff --git a/crypto/digest.c b/crypto/digest.c
index bc6de0b98f..b653fbb032 100644
--- a/crypto/digest.c
+++ b/crypto/digest.c
@@ -232,7 +232,7 @@ int digest_file_window(struct digest *d, const char *filename,
 	}
 
 	buf = memmap(fd, PROT_READ);
-	if (buf == (void *)-1) {
+	if (buf == MAP_FAILED) {
 		buf = xmalloc(4096);
 		flags = 1;
 	}
diff --git a/fs/fs.c b/fs/fs.c
index 625ed10b70..a304bf1863 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -533,7 +533,7 @@ void *memmap(int fd, int flags)
 {
 	struct fs_driver_d *fsdrv;
 	FILE *f;
-	void *retp = (void *)-1;
+	void *retp = MAP_FAILED;
 	int ret;
 
 	if (check_fd(fd))
diff --git a/include/fs.h b/include/fs.h
index 181318f404..f1514afa92 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -130,6 +130,8 @@ int protect(int fd, size_t count, loff_t offset, int prot);
 int protect_file(const char *file, int prot);
 void *memmap(int fd, int flags);
 
+#define MAP_FAILED ((void *)-1)
+
 #define FILESIZE_MAX	((loff_t)-1)
 
 #define PROT_READ	1
-- 
2.20.1


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

[PATCH 2/6] crypto: digest: Remove unused variable

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 crypto/digest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/crypto/digest.c b/crypto/digest.c
index b653fbb032..aff6d9876a 100644
--- a/crypto/digest.c
+++ b/crypto/digest.c
@@ -216,7 +216,6 @@ int digest_file_window(struct digest *d, const char *filename,
 		       const unsigned char *sig,
 		       ulong start, ulong size)
 {
-	ulong len = 0;
 	int fd, now, ret = 0;
 	unsigned char *buf;
 	int flags = 0;
@@ -271,7 +270,6 @@ int digest_file_window(struct digest *d, const char *filename,
 		if (ret)
 			goto out_free;
 		size -= now;
-		len += now;
 
 		if (!flags)
 			buf += now;
-- 
2.20.1


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

[PATCH 3/6] crypto: digest: Replace 4096 with PAGE_SIZE

[Andrey Smirnov]

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

diff --git a/crypto/digest.c b/crypto/digest.c
index aff6d9876a..980a694915 100644
--- a/crypto/digest.c
+++ b/crypto/digest.c
@@ -232,7 +232,7 @@ int digest_file_window(struct digest *d, const char *filename,
 
 	buf = memmap(fd, PROT_READ);
 	if (buf == MAP_FAILED) {
-		buf = xmalloc(4096);
+		buf = xmalloc(PAGE_SIZE);
 		flags = 1;
 	}
 
@@ -249,7 +249,7 @@ int digest_file_window(struct digest *d, const char *filename,
 	}
 
 	while (size) {
-		now = min((ulong)4096, size);
+		now = min((ulong)PAGE_SIZE, size);
 		if (flags) {
 			now = read(fd, buf, now);
 			if (now < 0) {
-- 
2.20.1


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

[PATCH 4/6] crypto: digest: Split memory vs. file code into separate functions

[Andrey Smirnov]

Instead of trying to fit two rather different cases into a single
loop, split digesting memory and digesting files into separate
subroutines. While duplicating some of the code shared by both of the
while() loops this makes the body of the loop easier to follow as well
as gets rid of poorly named "flags" variable.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 crypto/digest.c | 118 ++++++++++++++++++++++++++++--------------------
 1 file changed, 70 insertions(+), 48 deletions(-)

diff --git a/crypto/digest.c b/crypto/digest.c
index 980a694915..7b34742c52 100644
--- a/crypto/digest.c
+++ b/crypto/digest.c
@@ -211,14 +211,76 @@ void digest_free(struct digest *d)
 }
 EXPORT_SYMBOL_GPL(digest_free);
 
+static int digest_update_interruptible(struct digest *d, const void *data,
+				       unsigned long len)
+{
+	if (ctrlc())
+		return -EINTR;
+
+	return digest_update(d, data, len);
+}
+
+static int digest_update_from_fd(struct digest *d, int fd,
+				 ulong start, ulong size)
+{
+	unsigned char *buf = xmalloc(PAGE_SIZE);
+	int ret = 0;
+
+	ret = lseek(fd, start, SEEK_SET);
+	if (ret == -1) {
+		perror("lseek");
+		goto out_free;
+	}
+
+	while (size) {
+		const ssize_t now = read(fd, buf, PAGE_SIZE);
+		if (now < 0) {
+			ret = now;
+			perror("read");
+			goto out_free;
+		}
+
+		if (!now)
+			break;
+
+		ret = digest_update_interruptible(d, buf, now);
+		if (ret)
+			goto out_free;
+
+		size -= now;
+	}
+
+out_free:
+	free(buf);
+	return ret;
+}
+
+static int digest_update_from_memory(struct digest *d,
+				     const unsigned char *buf,
+				     ulong size)
+{
+	while (size) {
+		unsigned long now = min_t(typeof(size), PAGE_SIZE, size);
+		int ret;
+
+		ret = digest_update_interruptible(d, buf, now);
+		if (ret)
+			return ret;
+
+		size -= now;
+		buf  += now;
+	}
+
+	return 0;
+}
+
 int digest_file_window(struct digest *d, const char *filename,
 		       unsigned char *hash,
 		       const unsigned char *sig,
 		       ulong start, ulong size)
 {
-	int fd, now, ret = 0;
+	int fd, ret;
 	unsigned char *buf;
-	int flags = 0;
 
 	ret = digest_init(d);
 	if (ret)
@@ -231,58 +293,18 @@ int digest_file_window(struct digest *d, const char *filename,
 	}
 
 	buf = memmap(fd, PROT_READ);
-	if (buf == MAP_FAILED) {
-		buf = xmalloc(PAGE_SIZE);
-		flags = 1;
-	}
-
-	if (start > 0) {
-		if (flags) {
-			ret = lseek(fd, start, SEEK_SET);
-			if (ret == -1) {
-				perror("lseek");
-				goto out;
-			}
-		} else {
-			buf += start;
-		}
-	}
-
-	while (size) {
-		now = min((ulong)PAGE_SIZE, size);
-		if (flags) {
-			now = read(fd, buf, now);
-			if (now < 0) {
-				ret = now;
-				perror("read");
-				goto out_free;
-			}
-			if (!now)
-				break;
-		}
-
-		if (ctrlc()) {
-			ret = -EINTR;
-			goto out_free;
-		}
-
-		ret = digest_update(d, buf, now);
-		if (ret)
-			goto out_free;
-		size -= now;
+	if (buf == MAP_FAILED)
+		ret = digest_update_from_fd(d, fd, start, size);
+	else
+		ret = digest_update_from_memory(d, buf + start, size);
 
-		if (!flags)
-			buf += now;
-	}
+	if (ret)
+		goto out;
 
 	if (sig)
 		ret = digest_verify(d, sig);
 	else
 		ret = digest_final(d, hash);
-
-out_free:
-	if (flags)
-		free(buf);
 out:
 	close(fd);
 
-- 
2.20.1


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

Re: [PATCH 4/6] crypto: digest: Split memory vs. file code into separate functions

[Sam Ravnborg]

Hi Andrey.

> +static int digest_update_from_fd(struct digest *d, int fd,
> +				 ulong start, ulong size)
> +{
> +	unsigned char *buf = xmalloc(PAGE_SIZE);
> +	int ret = 0;
> +
> +	ret = lseek(fd, start, SEEK_SET);
> +	if (ret == -1) {
> +		perror("lseek");
> +		goto out_free;
> +	}
lseek return (off_t)-1 - should ret be of type off_t to make this correct?

The code looks more readable with the two helper functions.
So +1 from me.

	Sam

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

Re: [PATCH 4/6] crypto: digest: Split memory vs. file code into separate functions

[Andrey Smirnov]

On Sat, Jan 12, 2019 at 3:18 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Andrey.
>
> > +static int digest_update_from_fd(struct digest *d, int fd,
> > +                              ulong start, ulong size)
> > +{
> > +     unsigned char *buf = xmalloc(PAGE_SIZE);
> > +     int ret = 0;
> > +
> > +     ret = lseek(fd, start, SEEK_SET);
> > +     if (ret == -1) {
> > +             perror("lseek");
> > +             goto out_free;
> > +     }
> lseek return (off_t)-1 - should ret be of type off_t to make this correct?
>

I tried to avoid changing original code too much in this patch, just
to keep the scope of this patch to just refactoring. But yeah, I think
there's definitely a problem that a larger value of "start" passed to
lseek() will result in a failure due to "loff_t" downcast to "int".
Another problem with this is that lseek() doesn't really return a
detailed error code, so what this function should really do is capture
it from "errno" and this way "ret" can be kept as "int".

There's a couple of more places in digest.c that don't really capture
"errno", so I think I'll create a separate follow up series to fix
that, instead of re-spinning this one.

Thanks,
Andrey Smirnov


> The code looks more readable with the two helper functions.
> So +1 from me.
>
>         Sam

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

[PATCH 5/6] commands: digest: Use MAX_LFS_FILESIZE instead of ~0

[Andrey Smirnov]

Loff_t is a signed type, so ~0 is a negative number. While it works OK
due to clamping/conversion to ulong when passed to
digest_file_window(), it is better not to rely on that fact and use an
appropriate constant.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 commands/digest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/commands/digest.c b/commands/digest.c
index 0edbbec32c..99b27dcc25 100644
--- a/commands/digest.c
+++ b/commands/digest.c
@@ -6,6 +6,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <linux/pagemap.h>
 #include <fs.h>
 #include <fcntl.h>
 #include <errno.h>
@@ -35,7 +36,7 @@ int __do_digest(struct digest *d, unsigned char *sig,
 
 	while (*argv) {
 		char *filename = "/dev/mem";
-		loff_t start = 0, size = ~0;
+		loff_t start = 0, size = MAX_LFS_FILESIZE;
 		int show_area = 1;
 
 		/* arguments are either file, file+area or area */
-- 
2.20.1


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

[PATCH 6/6] crypto: digest: Change the signature of digest_file_window()

[Andrey Smirnov]

On 32-bit systems "ulong" will limit digest_file_window()'s maximum
size to 4 GiB. Convert "start" and "size" to "loff_t" in order to be
able to handle maximum file size supported by the rest of the system.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 crypto/digest.c  | 6 +++---
 include/digest.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/crypto/digest.c b/crypto/digest.c
index 7b34742c52..9b7b73019a 100644
--- a/crypto/digest.c
+++ b/crypto/digest.c
@@ -221,7 +221,7 @@ static int digest_update_interruptible(struct digest *d, const void *data,
 }
 
 static int digest_update_from_fd(struct digest *d, int fd,
-				 ulong start, ulong size)
+				 loff_t start, loff_t size)
 {
 	unsigned char *buf = xmalloc(PAGE_SIZE);
 	int ret = 0;
@@ -257,7 +257,7 @@ out_free:
 
 static int digest_update_from_memory(struct digest *d,
 				     const unsigned char *buf,
-				     ulong size)
+				     loff_t size)
 {
 	while (size) {
 		unsigned long now = min_t(typeof(size), PAGE_SIZE, size);
@@ -277,7 +277,7 @@ static int digest_update_from_memory(struct digest *d,
 int digest_file_window(struct digest *d, const char *filename,
 		       unsigned char *hash,
 		       const unsigned char *sig,
-		       ulong start, ulong size)
+		       loff_t start, loff_t size)
 {
 	int fd, ret;
 	unsigned char *buf;
diff --git a/include/digest.h b/include/digest.h
index a1cdbb2d7a..a87e29bd28 100644
--- a/include/digest.h
+++ b/include/digest.h
@@ -100,7 +100,7 @@ void digest_free(struct digest *d);
 int digest_file_window(struct digest *d, const char *filename,
 		       unsigned char *hash,
 		       const unsigned char *sig,
-		       ulong start, ulong size);
+		       loff_t start, loff_t size);
 int digest_file(struct digest *d, const char *filename,
 		unsigned char *hash,
 		const unsigned char *sig);
-- 
2.20.1


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

Re: [PATCH 0/6] Support for digesting large files (> 4 GiB)

[Sam Ravnborg]

Hi Andrey.


> If "crypto: digest: Split memory vs. file code
> into separate functions" is too controversial it can be dropped.
An improvement to readability if you ask me.

One nit, otherwise series looks good.

	Sam

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

Re: [PATCH 0/6] Support for digesting large files (> 4 GiB)

[Sascha Hauer]

On Sat, Jan 12, 2019 at 12:24:50AM -0800, Andrey Smirnov wrote:
> Everyone:
> 
> This is the series of patches I made while to fix incorrect behaviour
> while trying to md5sum a 128 GiB file. Most of the patches should be
> pretty straightforward. If "crypto: digest: Split memory vs. file code
> into separate functions" is too controversial it can be dropped.
> 
> Feedback is welcome!
> 
> Thanks,
> Andrey Smirnov
> 
> Andrey Smirnov (6):
>   treewide: Introduce MAP_FAILED and replace ad-hoc constants with it
>   crypto: digest: Remove unused variable
>   crypto: digest: Replace 4096 with PAGE_SIZE
>   crypto: digest: Split memory vs. file code into separate functions
>   commands: digest: Use MAX_LFS_FILESIZE instead of ~0
>   crypto: digest: Change the signature of digest_file_window()

Applied, thanks

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