Re: Kmap-related crashes and memory leaks on 32bit arch (5.15+)

[Qu Wenruo]

On 2021/11/5 07:48, Linus Torvalds wrote:
> On Thu, Nov 4, 2021 at 4:37 PM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > Having looked at it once more, it still looks "ObviouslyCorrect(tm)"
> > to me, but I suspect I'm just being blind to some obvious bug.
>
> Oh, I was just looking at the pattern of kmap/kunmap, but there does
> seem to be a questionable pattern outside of that:
>
> This pattern looks odd:
>
>          kaddr = kmap(cur_page);
>          write_compress_length(kaddr + offset_in_page(*cur_out),
>                                compressed_size);
>          ...
>
> (and then whether you kunmap immediately, or you leave it kmap'ed and
> use it again at the end is a different issue)

That part is paired with the the following code, to prevent we cross 
page boundary for the segment header:

	/*
	 * Check if we can fit the next segment header into the remaining space
	 * of the sector.
	 */
	sector_bytes_left = round_up(*cur_out, sectorsize) - *cur_out;
	if (sector_bytes_left >= LZO_LEN || sector_bytes_left == 0)
		goto out;

	/* The remaining size is not enough, pad it with zeros */
	memset(kaddr + offset_in_page(*cur_out), 0,
	       sector_bytes_left);
	*cur_out += sector_bytes_left;


So we always ensure that each segment header never crosses the page 
boundary.

This behavior is a little tricky but is part of the on-disk format for 
lzo compressed data.


BTW, I also thought that part can be suspicious, as it keeps the page 
mapped (unlike all other call sites), thus I tried the following diff, 
but no difference for the leakage:

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 65cb0766e62d..0648acc48310 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -151,6 +151,7 @@ static int copy_compressed_data_to_page(char 
*compressed_data,
 	kaddr = kmap(cur_page);
 	write_compress_length(kaddr + offset_in_page(*cur_out),
 			      compressed_size);
+	kunmap(cur_page);
 	*cur_out += LZO_LEN;

 	orig_out = *cur_out;
@@ -160,7 +161,6 @@ static int copy_compressed_data_to_page(char 
*compressed_data,
 		u32 copy_len = min_t(u32, sectorsize - *cur_out % sectorsize,
 				     orig_out + compressed_size - *cur_out);

-		kunmap(cur_page);
 		cur_page = out_pages[*cur_out / PAGE_SIZE];
 		/* Allocate a new page */
 		if (!cur_page) {
@@ -173,6 +173,7 @@ static int copy_compressed_data_to_page(char 
*compressed_data,

 		memcpy(kaddr + offset_in_page(*cur_out),
 		       compressed_data + *cur_out - orig_out, copy_len);
+		kunmap(cur_page);

 		*cur_out += copy_len;
 	}
@@ -186,12 +187,15 @@ static int copy_compressed_data_to_page(char 
*compressed_data,
 		goto out;

 	/* The remaining size is not enough, pad it with zeros */
+	cur_page = out_pages[*cur_out / PAGE_SIZE];
+	ASSERT(cur_page);
+	kmap(cur_page);
 	memset(kaddr + offset_in_page(*cur_out), 0,
 	       sector_bytes_left);
+	kunmap(cur_page);
 	*cur_out += sector_bytes_left;

 out:
-	kunmap(cur_page);
 	return 0;
 }

Thanks,
Qu
> In particular, what if "offset_in_page(*cur_out)" is very close to the
> end of the page?
>
> That write_compress_length() always writes out a word-sized length (ie
> LZO_LEN bytes), and the above pattern seems to have no model to handle
> the "oh, this 4-byte write crosses a page boundary"
>
> The other writes in that function seem to do it properly, and you have
>
>          u32 copy_len = min_t(u32, sectorsize - *cur_out % sectorsize,
>                               orig_out + compressed_size - *cur_out);
>
> so doing the memcpy() of size 'copy_len' should never cross a page
> boundary as long as sectorsize is a power-of-2 smaller or equal to a
> page size. But those 4-byte length writes seem like they could be page
> crossers.
>
> The same situation exists on the reading side, I think.
>
> Maybe there's some reason why the read/write_compress_length() can
> never cross a page boundary, but it did strike me as odd.
>
>               Linus