mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] fs: jffs2: fix error when reading blocks with offset
@ 2021-12-02 10:48 Holger Assmann
  2021-12-03  6:37 ` Ahmad Fatoum
  2021-12-07  8:13 ` Sascha Hauer
  0 siblings, 2 replies; 3+ messages in thread
From: Holger Assmann @ 2021-12-02 10:48 UTC (permalink / raw)
  To: barebox; +Cc: afa, Holger Assmann

This bug resulted in a panic when trying to read a file partially with
an offset, e.g. when starting Linux. In such a case a header is loaded
by a separate call of jffs2_read(), which then copies only the first n
bytes out of the respective block.
When the remaining data of the block is then subsequently going to to be
read the system deviates in its behaviour from that under Linux by not
calling jffs2_read_inode_range() in a 4k-alignment, but with an offset.

jffs2_read_inode_range() originates from the Linux jffs2 driver. When
being called with an offset it still reads 4096 bytes of data and
eventually returns fragments of two consecutive blocks. jffs2_read()
then reads this result whilst again applying the offset, therefore
returning faulty data.

We fix that problem by calling jffs2_get_block() without an offset and
therefore reading the whole block. The offset is then applied when we
actually perform memcpy with the returned buffer. This fix might also
increase the performance since the respective block is likely to be
cached from the previous call.

Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
---
 fs/jffs2/fs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 1a2b443792..3d4cb98c97 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -74,6 +74,9 @@ static int jffs2_get_block(struct jffs2_file *jf, unsigned int pos)
 	struct jffs2_inode_info *f = JFFS2_INODE_INFO(jf->inode);
 	int ret;
 
+	/* pos always has to be 4096 bytes aligned here */
+	WARN_ON(pos % 4096 != 0);
+
 	if (pos != jf->offset) {
 		ret = jffs2_read_inode_range(c, f, jf->buf, pos,
 					     JFFS2_BLOCK_SIZE);
@@ -98,13 +101,14 @@ static int jffs2_read(struct device_d *_dev, FILE *f, void *buf,
 	/* Read till end of current block */
 	ofs = f->pos % JFFS2_BLOCK_SIZE;
 	if (ofs) {
-		ret = jffs2_get_block(jf, pos);
+		ret = jffs2_get_block(jf, f->pos - ofs); /* Align down block */
 		if (ret)
 			return ret;
 
 		now = min(size, JFFS2_BLOCK_SIZE - ofs);
 
-		memcpy(buf, jf->buf + ofs, now);
+		/* Complete block has been read, re-apply ofset now */
+		memcpy(buf, jf->buf + ofs, now); 
 		size -= now;
 		pos += now;
 		buf += now;
-- 
2.30.2


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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] fs: jffs2: fix error when reading blocks with offset
  2021-12-02 10:48 [PATCH] fs: jffs2: fix error when reading blocks with offset Holger Assmann
@ 2021-12-03  6:37 ` Ahmad Fatoum
  2021-12-07  8:13 ` Sascha Hauer
  1 sibling, 0 replies; 3+ messages in thread
From: Ahmad Fatoum @ 2021-12-03  6:37 UTC (permalink / raw)
  To: Holger Assmann, barebox; +Cc: afa

On 02.12.21 11:48, Holger Assmann wrote:
> This bug resulted in a panic when trying to read a file partially with
> an offset, e.g. when starting Linux. In such a case a header is loaded
> by a separate call of jffs2_read(), which then copies only the first n
> bytes out of the respective block.
> When the remaining data of the block is then subsequently going to to be
> read the system deviates in its behaviour from that under Linux by not
> calling jffs2_read_inode_range() in a 4k-alignment, but with an offset.
> 
> jffs2_read_inode_range() originates from the Linux jffs2 driver. When
> being called with an offset it still reads 4096 bytes of data and
> eventually returns fragments of two consecutive blocks. jffs2_read()
> then reads this result whilst again applying the offset, therefore
> returning faulty data.
> 
> We fix that problem by calling jffs2_get_block() without an offset and
> therefore reading the whole block. The offset is then applied when we
> actually perform memcpy with the returned buffer. This fix might also
> increase the performance since the respective block is likely to be
> cached from the previous call.
> 
> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> ---
>  fs/jffs2/fs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index 1a2b443792..3d4cb98c97 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -74,6 +74,9 @@ static int jffs2_get_block(struct jffs2_file *jf, unsigned int pos)
>  	struct jffs2_inode_info *f = JFFS2_INODE_INFO(jf->inode);
>  	int ret;
>  
> +	/* pos always has to be 4096 bytes aligned here */
> +	WARN_ON(pos % 4096 != 0);
> +
>  	if (pos != jf->offset) {
>  		ret = jffs2_read_inode_range(c, f, jf->buf, pos,
>  					     JFFS2_BLOCK_SIZE);
> @@ -98,13 +101,14 @@ static int jffs2_read(struct device_d *_dev, FILE *f, void *buf,
>  	/* Read till end of current block */
>  	ofs = f->pos % JFFS2_BLOCK_SIZE;
>  	if (ofs) {
> -		ret = jffs2_get_block(jf, pos);
> +		ret = jffs2_get_block(jf, f->pos - ofs); /* Align down block */
>  		if (ret)
>  			return ret;
>  
>  		now = min(size, JFFS2_BLOCK_SIZE - ofs);
>  
> -		memcpy(buf, jf->buf + ofs, now);
> +		/* Complete block has been read, re-apply ofset now */
> +		memcpy(buf, jf->buf + ofs, now); 

Newly added stray space at end of line.

Apart from that:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

>  		size -= now;
>  		pos += now;
>  		buf += now;
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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] 3+ messages in thread

* Re: [PATCH] fs: jffs2: fix error when reading blocks with offset
  2021-12-02 10:48 [PATCH] fs: jffs2: fix error when reading blocks with offset Holger Assmann
  2021-12-03  6:37 ` Ahmad Fatoum
@ 2021-12-07  8:13 ` Sascha Hauer
  1 sibling, 0 replies; 3+ messages in thread
From: Sascha Hauer @ 2021-12-07  8:13 UTC (permalink / raw)
  To: Holger Assmann; +Cc: barebox, afa

On Thu, Dec 02, 2021 at 11:48:37AM +0100, Holger Assmann wrote:
> This bug resulted in a panic when trying to read a file partially with
> an offset, e.g. when starting Linux. In such a case a header is loaded
> by a separate call of jffs2_read(), which then copies only the first n
> bytes out of the respective block.
> When the remaining data of the block is then subsequently going to to be
> read the system deviates in its behaviour from that under Linux by not
> calling jffs2_read_inode_range() in a 4k-alignment, but with an offset.
> 
> jffs2_read_inode_range() originates from the Linux jffs2 driver. When
> being called with an offset it still reads 4096 bytes of data and
> eventually returns fragments of two consecutive blocks. jffs2_read()
> then reads this result whilst again applying the offset, therefore
> returning faulty data.
> 
> We fix that problem by calling jffs2_get_block() without an offset and
> therefore reading the whole block. The offset is then applied when we
> actually perform memcpy with the returned buffer. This fix might also
> increase the performance since the respective block is likely to be
> cached from the previous call.
> 
> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> ---
>  fs/jffs2/fs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index 1a2b443792..3d4cb98c97 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -74,6 +74,9 @@ static int jffs2_get_block(struct jffs2_file *jf, unsigned int pos)
>  	struct jffs2_inode_info *f = JFFS2_INODE_INFO(jf->inode);
>  	int ret;
>  
> +	/* pos always has to be 4096 bytes aligned here */
> +	WARN_ON(pos % 4096 != 0);

This should be JFFS2_BLOCK_SIZE, right?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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] 3+ messages in thread

end of thread, other threads:[~2021-12-07  8:55 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-02 10:48 [PATCH] fs: jffs2: fix error when reading blocks with offset Holger Assmann
2021-12-03  6:37 ` Ahmad Fatoum
2021-12-07  8:13 ` Sascha Hauer

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