* [PATCH 1/3] squashfs: fix read regression introduced in readahead code
2022-10-20 22:36 [0/3] squashfs: fix some regressions introduced in the readahead code Phillip Lougher
@ 2022-10-20 22:36 ` Phillip Lougher
2022-10-20 22:36 ` [PATCH 2/3] squashfs: fix extending readahead beyond end of file Phillip Lougher
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Phillip Lougher @ 2022-10-20 22:36 UTC (permalink / raw)
To: linux-kernel, akpm
Cc: hsinyi, regressions, regressions, dimitri.ledkov, michael.vogt,
phillip.lougher, ogra, olivier.tilloy, Phillip Lougher,
Mirsad Goran Todorovac, Slade Watkins, Bagas Sanjaya, stable
If a file isn't a whole multiple of the page size, the last page will
have trailing bytes unfilled.
There was a mistake in the readahead code which did this. In
particular it incorrectly assumed that the last page in the
readahead page array (page[nr_pages - 1]) will always contain the
last page in the block, which if we're at file end, will be the page
that needs to be zero filled.
But the readahead code may not return the last page in the block, which
means it is unmapped and will be skipped by the decompressors (a
temporary buffer used).
In this case the zero filling code will zero out the wrong page, leading
to data corruption.
Fix this by by extending the "page actor" to return the last page if
present, or NULL if a temporary buffer was used.
Fixes: 8fc78b6fe24c ("squashfs: implement readahead")
Link: https://lore.kernel.org/lkml/b0c258c3-6dcf-aade-efc4-d62a8b3a1ce2@alu.unizg.hr/
Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
Tested-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
Tested-by: Slade Watkins <srw@sladewatkins.net>
Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
---
fs/squashfs/file.c | 7 ++++---
fs/squashfs/page_actor.c | 3 +++
fs/squashfs/page_actor.h | 6 +++++-
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index e56510964b22..e526eb7a1658 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -557,6 +557,7 @@ static void squashfs_readahead(struct readahead_control *ractl)
int res, bsize;
u64 block = 0;
unsigned int expected;
+ struct page *last_page;
nr_pages = __readahead_batch(ractl, pages, max_pages);
if (!nr_pages)
@@ -593,15 +594,15 @@ static void squashfs_readahead(struct readahead_control *ractl)
res = squashfs_read_data(inode->i_sb, block, bsize, NULL, actor);
- squashfs_page_actor_free(actor);
+ last_page = squashfs_page_actor_free(actor);
if (res == expected) {
int bytes;
/* Last page (if present) may have trailing bytes not filled */
bytes = res % PAGE_SIZE;
- if (pages[nr_pages - 1]->index == file_end && bytes)
- memzero_page(pages[nr_pages - 1], bytes,
+ if (index == file_end && bytes && last_page)
+ memzero_page(last_page, bytes,
PAGE_SIZE - bytes);
for (i = 0; i < nr_pages; i++) {
diff --git a/fs/squashfs/page_actor.c b/fs/squashfs/page_actor.c
index 54b93bf4a25c..81af6c4ca115 100644
--- a/fs/squashfs/page_actor.c
+++ b/fs/squashfs/page_actor.c
@@ -71,11 +71,13 @@ static void *handle_next_page(struct squashfs_page_actor *actor)
(actor->next_index != actor->page[actor->next_page]->index)) {
actor->next_index++;
actor->returned_pages++;
+ actor->last_page = NULL;
return actor->alloc_buffer ? actor->tmp_buffer : ERR_PTR(-ENOMEM);
}
actor->next_index++;
actor->returned_pages++;
+ actor->last_page = actor->page[actor->next_page];
return actor->pageaddr = kmap_local_page(actor->page[actor->next_page++]);
}
@@ -125,6 +127,7 @@ struct squashfs_page_actor *squashfs_page_actor_init_special(struct squashfs_sb_
actor->returned_pages = 0;
actor->next_index = page[0]->index & ~((1 << (msblk->block_log - PAGE_SHIFT)) - 1);
actor->pageaddr = NULL;
+ actor->last_page = NULL;
actor->alloc_buffer = msblk->decompressor->alloc_buffer;
actor->squashfs_first_page = direct_first_page;
actor->squashfs_next_page = direct_next_page;
diff --git a/fs/squashfs/page_actor.h b/fs/squashfs/page_actor.h
index 95ffbb543d91..97d4983559b1 100644
--- a/fs/squashfs/page_actor.h
+++ b/fs/squashfs/page_actor.h
@@ -16,6 +16,7 @@ struct squashfs_page_actor {
void *(*squashfs_first_page)(struct squashfs_page_actor *);
void *(*squashfs_next_page)(struct squashfs_page_actor *);
void (*squashfs_finish_page)(struct squashfs_page_actor *);
+ struct page *last_page;
int pages;
int length;
int next_page;
@@ -29,10 +30,13 @@ extern struct squashfs_page_actor *squashfs_page_actor_init(void **buffer,
extern struct squashfs_page_actor *squashfs_page_actor_init_special(
struct squashfs_sb_info *msblk,
struct page **page, int pages, int length);
-static inline void squashfs_page_actor_free(struct squashfs_page_actor *actor)
+static inline struct page *squashfs_page_actor_free(struct squashfs_page_actor *actor)
{
+ struct page *last_page = actor->last_page;
+
kfree(actor->tmp_buffer);
kfree(actor);
+ return last_page;
}
static inline void *squashfs_first_page(struct squashfs_page_actor *actor)
{
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] squashfs: fix extending readahead beyond end of file
2022-10-20 22:36 [0/3] squashfs: fix some regressions introduced in the readahead code Phillip Lougher
2022-10-20 22:36 ` [PATCH 1/3] squashfs: fix read regression introduced in " Phillip Lougher
@ 2022-10-20 22:36 ` Phillip Lougher
2022-10-20 22:36 ` [PATCH 3/3] squashfs: fix buffer release race condition in readahead code Phillip Lougher
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Phillip Lougher @ 2022-10-20 22:36 UTC (permalink / raw)
To: linux-kernel, akpm
Cc: hsinyi, regressions, regressions, dimitri.ledkov, michael.vogt,
phillip.lougher, ogra, olivier.tilloy, Phillip Lougher, stable
The readahead code will try to extend readahead to the entire
size of the Squashfs data block.
But, it didn't take into account that the last block at the end of
the file may not be a whole block. In this case, the code would
extend readahead to beyond the end of the file, leaving trailing
pages.
Fix this by only requesting the expected number of pages.
Fixes: 8fc78b6fe24c ("squashfs: implement readahead")
Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
Cc: <stable@vger.kernel.org>
---
fs/squashfs/file.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index e526eb7a1658..f0afd4d6fd30 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -559,6 +559,12 @@ static void squashfs_readahead(struct readahead_control *ractl)
unsigned int expected;
struct page *last_page;
+ expected = start >> msblk->block_log == file_end ?
+ (i_size_read(inode) & (msblk->block_size - 1)) :
+ msblk->block_size;
+
+ max_pages = (expected + PAGE_SIZE - 1) >> PAGE_SHIFT;
+
nr_pages = __readahead_batch(ractl, pages, max_pages);
if (!nr_pages)
break;
@@ -567,13 +573,10 @@ static void squashfs_readahead(struct readahead_control *ractl)
goto skip_pages;
index = pages[0]->index >> shift;
+
if ((pages[nr_pages - 1]->index >> shift) != index)
goto skip_pages;
- expected = index == file_end ?
- (i_size_read(inode) & (msblk->block_size - 1)) :
- msblk->block_size;
-
if (index == file_end && squashfs_i(inode)->fragment_block !=
SQUASHFS_INVALID_BLK) {
res = squashfs_readahead_fragment(pages, nr_pages,
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] squashfs: fix buffer release race condition in readahead code
2022-10-20 22:36 [0/3] squashfs: fix some regressions introduced in the readahead code Phillip Lougher
2022-10-20 22:36 ` [PATCH 1/3] squashfs: fix read regression introduced in " Phillip Lougher
2022-10-20 22:36 ` [PATCH 2/3] squashfs: fix extending readahead beyond end of file Phillip Lougher
@ 2022-10-20 22:36 ` Phillip Lougher
2022-10-21 12:42 ` [0/3] squashfs: fix some regressions introduced in the " Bagas Sanjaya
[not found] ` <4493e774-ff23-e2a1-4aa7-28b190eb909d@alu.unizg.hr>
4 siblings, 0 replies; 6+ messages in thread
From: Phillip Lougher @ 2022-10-20 22:36 UTC (permalink / raw)
To: linux-kernel, akpm
Cc: hsinyi, regressions, regressions, dimitri.ledkov, michael.vogt,
phillip.lougher, ogra, olivier.tilloy, Phillip Lougher, stable
Fix a buffer release race condition, where the error value was
used after release.
Fixes: b09a7a036d20 ("squashfs: support reading fragments in readahead call")
Cc: <stable@vger.kernel.org>
Signed-off-by: Phillip Lougher <phillip@squashfs.org.uk>
---
fs/squashfs/file.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/squashfs/file.c b/fs/squashfs/file.c
index f0afd4d6fd30..8ba8c4c50770 100644
--- a/fs/squashfs/file.c
+++ b/fs/squashfs/file.c
@@ -506,8 +506,9 @@ static int squashfs_readahead_fragment(struct page **page,
squashfs_i(inode)->fragment_size);
struct squashfs_sb_info *msblk = inode->i_sb->s_fs_info;
unsigned int n, mask = (1 << (msblk->block_log - PAGE_SHIFT)) - 1;
+ int error = buffer->error;
- if (buffer->error)
+ if (error)
goto out;
expected += squashfs_i(inode)->fragment_offset;
@@ -529,7 +530,7 @@ static int squashfs_readahead_fragment(struct page **page,
out:
squashfs_cache_put(buffer);
- return buffer->error;
+ return error;
}
static void squashfs_readahead(struct readahead_control *ractl)
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [0/3] squashfs: fix some regressions introduced in the readahead code
2022-10-20 22:36 [0/3] squashfs: fix some regressions introduced in the readahead code Phillip Lougher
` (2 preceding siblings ...)
2022-10-20 22:36 ` [PATCH 3/3] squashfs: fix buffer release race condition in readahead code Phillip Lougher
@ 2022-10-21 12:42 ` Bagas Sanjaya
[not found] ` <4493e774-ff23-e2a1-4aa7-28b190eb909d@alu.unizg.hr>
4 siblings, 0 replies; 6+ messages in thread
From: Bagas Sanjaya @ 2022-10-21 12:42 UTC (permalink / raw)
To: Phillip Lougher
Cc: linux-kernel, akpm, hsinyi, regressions, regressions,
dimitri.ledkov, michael.vogt, phillip.lougher, ogra,
olivier.tilloy
[-- Attachment #1: Type: text/plain, Size: 971 bytes --]
On Thu, Oct 20, 2022 at 11:36:13PM +0100, Phillip Lougher wrote:
> Hi,
>
> This patch-set fixes 3 regressions introduced by the recent readahead code.
> The first regression is causing "snaps" to randomly fail after a couple of
> hours or days, which how the regression came to light.
>
> ----------------------------------------------------------------
> Phillip Lougher (3):
> squashfs: fix read regression introduced in readahead code
> squashfs: fix extending readahead beyond end of file
> squashfs: fix buffer release race condition in readahead code
>
> fs/squashfs/file.c | 23 ++++++++++++++---------
> fs/squashfs/page_actor.c | 3 +++
> fs/squashfs/page_actor.h | 6 +++++-
> 3 files changed, 22 insertions(+), 10 deletions(-)
The regression goes away with this series, thanks.
For the series,
Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <4493e774-ff23-e2a1-4aa7-28b190eb909d@alu.unizg.hr>]
* Re: [0/3] squashfs: fix some regressions introduced in the readahead code
[not found] ` <4493e774-ff23-e2a1-4aa7-28b190eb909d@alu.unizg.hr>
@ 2022-10-22 22:56 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2022-10-22 22:56 UTC (permalink / raw)
To: Mirsad Goran Todorovac
Cc: Phillip Lougher, linux-kernel, hsinyi, regressions, regressions,
dimitri.ledkov, michael.vogt, phillip.lougher, ogra,
olivier.tilloy, Marc Miltenberger
On Sat, 22 Oct 2022 03:38:01 +0200 Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> wrote:
> Please consider crediting Marc Miltenberger, who was first to reproduce
> and confirm that the
> bug was not in my build but in mainline kernel.
Thanks, Marc. I've added your Reported-by: to my copies of the
three patches.
^ permalink raw reply [flat|nested] 6+ messages in thread