[PATCH] commands: mem: truncate mem device size to fit the loff_t file size

[PATCH] commands: mem: truncate mem device size to fit the loff_t file size

[Lucas Stach]

On 64bit arches the file covering the whole address space is larger than
what can be represented in the loff_t type (s64) used for the file size.
Thus the size of this device is interpreted as negative in a lot of
places. Fix this by truncating the size to fit the file size type.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 commands/mem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/commands/mem.c b/commands/mem.c
index eb91ade05a88..cdd7a492d0d5 100644
--- a/commands/mem.c
+++ b/commands/mem.c
@@ -96,7 +96,8 @@ static int mem_probe(struct device_d *dev)
 	dev->priv = cdev;
 
 	cdev->name = (char*)dev->resource[0].name;
-	cdev->size = (unsigned long)resource_size(&dev->resource[0]);
+	cdev->size = min(resource_size(&dev->resource[0]),
+			 (unsigned long long)S64_MAX);
 	cdev->ops = &memops;
 	cdev->dev = dev;
 
-- 
2.19.1


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

Re: [PATCH] commands: mem: truncate mem device size to fit the loff_t file size

[Sascha Hauer]

On Tue, Nov 06, 2018 at 05:14:07PM +0100, Lucas Stach wrote:
> On 64bit arches the file covering the whole address space is larger than
> what can be represented in the loff_t type (s64) used for the file size.
> Thus the size of this device is interpreted as negative in a lot of
> places. Fix this by truncating the size to fit the file size type.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  commands/mem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Applied, thanks

Sascha

> 
> diff --git a/commands/mem.c b/commands/mem.c
> index eb91ade05a88..cdd7a492d0d5 100644
> --- a/commands/mem.c
> +++ b/commands/mem.c
> @@ -96,7 +96,8 @@ static int mem_probe(struct device_d *dev)
>  	dev->priv = cdev;
>  
>  	cdev->name = (char*)dev->resource[0].name;
> -	cdev->size = (unsigned long)resource_size(&dev->resource[0]);
> +	cdev->size = min(resource_size(&dev->resource[0]),
> +			 (unsigned long long)S64_MAX);
>  	cdev->ops = &memops;
>  	cdev->dev = dev;
>  
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
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

Re: [PATCH] commands: mem: truncate mem device size to fit the loff_t file size

[Peter Mamonov]

Hi, Lucas,

On Tue, Nov 06, 2018 at 05:14:07PM +0100, Lucas Stach wrote:
> On 64bit arches the file covering the whole address space is larger than
> what can be represented in the loff_t type (s64) used for the file size.
> Thus the size of this device is interpreted as negative in a lot of
> places. Fix this by truncating the size to fit the file size type.

While this is probably an appropriate solution for some architectures, it will 
cause problems on MIPS64. The root of evil is MIPS64 address space 
segmentation: https://docplayer.net/docs-images/25/4553906/images/21-0.png).  
Most notably your patch prevents access to the xkphys segment, where most 
peripheral devices of a SoC are accessible.

Regards,
Peter

> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  commands/mem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/commands/mem.c b/commands/mem.c
> index eb91ade05a88..cdd7a492d0d5 100644
> --- a/commands/mem.c
> +++ b/commands/mem.c
> @@ -96,7 +96,8 @@ static int mem_probe(struct device_d *dev)
>  	dev->priv = cdev;
>  
>  	cdev->name = (char*)dev->resource[0].name;
> -	cdev->size = (unsigned long)resource_size(&dev->resource[0]);
> +	cdev->size = min(resource_size(&dev->resource[0]),
> +			 (unsigned long long)S64_MAX);
>  	cdev->ops = &memops;
>  	cdev->dev = dev;
>  
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

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

Re: [PATCH] commands: mem: truncate mem device size to fit the loff_t file size

[Lucas Stach]

Am Mittwoch, den 07.11.2018, 16:45 +0300 schrieb Peter Mamonov:
> Hi, Lucas,
> 
> On Tue, Nov 06, 2018 at 05:14:07PM +0100, Lucas Stach wrote:
> > On 64bit arches the file covering the whole address space is larger than
> > what can be represented in the loff_t type (s64) used for the file size.
> > Thus the size of this device is interpreted as negative in a lot of
> > places. Fix this by truncating the size to fit the file size type.
> 
> While this is probably an appropriate solution for some architectures, it will 
> cause problems on MIPS64. The root of evil is MIPS64 address space 
> segmentation: https://docplayer.net/docs-images/25/4553906/images/21-0.png).  
> Most notably your patch prevents access to the xkphys segment, where most 
> peripheral devices of a SoC are accessible.

Isn't this a problem for MIPS64 even before my patch? For example the
"mw" command uses open_and_lseek on the dev/mem file with the address
of the write being the seek offset. As this is a loff_t value, seeking
to something with the highest order bit set won't do what you expect to
happen, right?

Regards,
Lucas

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

Re: [PATCH] commands: mem: truncate mem device size to fit the loff_t file size

[Peter Mamonov]

On Wed, Nov 07, 2018 at 05:08:17PM +0100, Lucas Stach wrote:
> Am Mittwoch, den 07.11.2018, 16:45 +0300 schrieb Peter Mamonov:
> > Hi, Lucas,
> > 
> > On Tue, Nov 06, 2018 at 05:14:07PM +0100, Lucas Stach wrote:
> > > On 64bit arches the file covering the whole address space is larger than
> > > what can be represented in the loff_t type (s64) used for the file size.
> > > Thus the size of this device is interpreted as negative in a lot of
> > > places. Fix this by truncating the size to fit the file size type.
> > 
> > While this is probably an appropriate solution for some architectures, it will 
> > cause problems on MIPS64. The root of evil is MIPS64 address space 
> > segmentation: https://docplayer.net/docs-images/25/4553906/images/21-0.png).  
> > Most notably your patch prevents access to the xkphys segment, where most 
> > peripheral devices of a SoC are accessible.
> 
> Isn't this a problem for MIPS64 even before my patch? For example the
> "mw" command uses open_and_lseek on the dev/mem file with the address
> of the write being the seek offset. As this is a loff_t value, seeking
> to something with the highest order bit set won't do what you expect to
> happen, right?

You are right. I fixed this in a straightforward manner (see the patch 
below). Guess it's time to find a better solution :)

	Author: Peter Mamonov <pmamonov@gmail.com>
	Date:   Thu Jul 6 13:32:53 2017 +0300
	
	    FIXME: fs: fix memory access for 64bit MIPS
	
	diff --git a/fs/fs.c b/fs/fs.c
	index f61ee091b5..45190812c5 100644
	--- a/fs/fs.c
	+++ b/fs/fs.c
	@@ -941,8 +941,6 @@ loff_t lseek(int fildes, loff_t offset, int whence)
		case SEEK_SET:
			if (f->size != FILE_SIZE_STREAM && offset > f->size)
				goto out;
	-               if (offset < 0)
	-                       goto out;
			pos = offset;
			break;
		case SEEK_CUR:
	@@ -960,10 +958,6 @@ loff_t lseek(int fildes, loff_t offset, int whence)
		}
	        
		pos = fsdrv->lseek(&f->fsdev->dev, f, pos);
	-       if (pos < 0) {
	-               errno = -pos;
	-               return -1;
	-       }
	        
		return pos;

Regards,
Peter

> 
> Regards,
> Lucas

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