Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <f3d3dc5d-dcf8-76b7-f383-aed3c942ae49@suse.com>
Date:   Fri, 5 Nov 2021 08:01:13 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Content-Language: en-US
To:     Linus Torvalds <torvalds@linux-foundation.org>,
        David Sterba <dsterba@suse.cz>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>
References: <20211104115001.GU20319@twin.jikos.cz>
 <CAHk-=whYQvExYESEOJoSj4Jy7t+tSZgbCWuNpdwXYh+3zq2itw@mail.gmail.com>
 <CAHk-=whBOXM3mh-QtzK-EQtDEHQLcziAXu07KxU1crUc5jiQUg@mail.gmail.com>
 <CAHk-=whGUxtcL8Z67y4A6_diSmtQdnOq1p_gyBAMzpKD9yk+gw@mail.gmail.com>
From:   Qu Wenruo <wqu@suse.com>
Subject: Re: Kmap-related crashes and memory leaks on 32bit arch (5.15+)
In-Reply-To: <CAHk-=whGUxtcL8Z67y4A6_diSmtQdnOq1p_gyBAMzpKD9yk+gw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QUlJMjdWQ2YrdWFlTTJaTnplWVVMeTUvUWlyRmliYkFWZzRiRG02QlE5WGQ1?=
 =?utf-8?B?T09Ma085NHJxNm1jYkJvMGxHZ0NBby9Eb0VpYVBYd3ZYSXMzdnBuNzFhNHQw?=
 =?utf-8?B?UlRzcGFmWCt1VmpxMXM1UTVZVjhYNEhsZ0g0bnkrV3NFYVh4VHlETHh2T0NW?=
 =?utf-8?B?OExPK0d5enU1SE9HU0tpRVhhekpRNFVBVldjV2RLZGNpTEFSMDFmUDl1UCsw?=
 =?utf-8?B?eUJHaExOT0lpelFRV2ZXcXBwTGx0enVsVVNkRmNtL0JPR0x3OXRBR2EvdUNk?=
 =?utf-8?B?SzBTL2JtcWhkcWtjclBzejNZV2x0V01tNHozc0sxa2d3RkZUbXQrWkRRRUVR?=
 =?utf-8?B?c3dmaWtqcTFOSXAvdVVSaGRoakVxd2QzVE5pUVBOR0d0dUF1U0NYSEZkQW54?=
 =?utf-8?B?M0dLdlpybUxzYmVyLy9tazg2b1BWaEJXQk9vTU03Ty95TUVGNUFmeE12S2xF?=
 =?utf-8?B?dVc0MC9ockhEQ1JKL3BXVGlUZTBqVmt6V3JlOGM5bkNydDYrNGY5OVM4aits?=
 =?utf-8?B?YnJubEdYUEZvaTZjdE1UVHlURTYwYUlkR0dzMXRZajl4aUdsVVV5SllNSnMy?=
 =?utf-8?B?RzhvL2tpV3ZvU0dnSEZvZWhIZm1kdkVaOTNiR05vaW5WY0o1U291b0Jra2g0?=
 =?utf-8?B?S2NwTWtYTVFJYzl5cVRPRE5rYyswR0NyN2x4UnZtRk5XaTdCVkdYcGR2ekQx?=
 =?utf-8?B?TWx1Qk4zVHludzFuLzZCWWFwbVFKdXFoYmFINmNhZ0NVTGRoWU41aXJDWmhI?=
 =?utf-8?B?UDg2eDFZVThaNlpOWVJFdnFzbkMwSVNrSzA5cnp3QTB3YjZVVnJxK3RzNmV2?=
 =?utf-8?B?YWZNMS8rd0VrcjZwR2dGMzNBNUozc3hoalIyd2Q3WkFDVzlZZ0ZybW95MVNC?=
 =?utf-8?B?K0QxM2VrNmYvZnBNSzJ2U0NSem9EWjhHc3RUT3RFRjIvWUJ5cHQyc09ZTWJ6?=
 =?utf-8?B?MWsyTzE2ZWZLWWtpS1llaFB4bjMxNi9GUjFiblZGaElVVGU0ZXJIS2xQOVk0?=
 =?utf-8?B?YWtsT2ZVaFArQmRUWXBrZkIyQ0Yvd3J0a3BORG1sVndFSjFreE50QlE2WFVT?=
 =?utf-8?B?VGpkbVhYT3FqbFd0Um1aTWd4TE9iSm1EaEd2SVlYejlVSjZZaW1ZcWp2cXhM?=
 =?utf-8?B?aUpmQ095VXI5Vm5GK05tU0RrN09tQWpwd0pDQzhWWEZQTk9BQ0FvTnNPQTRX?=
 =?utf-8?B?QVZ5dk00WDdNNFJrM3pteE1CWVlXK1Q5RFNyTWJUL2VCRTVSTEI1QUJyeG8z?=
 =?utf-8?B?VURWU0dSUlo1VEIzLzNVbU5VMDUyUy8yaWg0ZmErSFY2cUNlZzgwczgyVjlz?=
 =?utf-8?B?cmVQOGdaOE9YTzdzbTZvdHIyQk1tRzJvRHgwZHJhamZHSU1PK25ZeWVqOXJN?=
 =?utf-8?B?MGVlNXdQK3FKdE0wMUdlWm52d3ByVWN2N2RPd0tWNWY0SXJ1cHdPdkcrK2F1?=
 =?utf-8?B?aTRWMGNhZXlhWE4rMjJaZnF4a1RxaGZ5MlJVVHRKQTJ3U21xZ3QreW1CWE5o?=
 =?utf-8?B?UzVXQkFodTRLY0k5ZHB1K0F2RnlINHFRZmd2bEdhS29Zd0FWTWduVHZPa1NR?=
 =?utf-8?B?aXlBWlpjSVp4OTV1emw2T01OY3o4M2FERmZINDU0UVF0NlVZekpHbDhMS3pa?=
 =?utf-8?B?TndHbUJ5R2Rhd0ZhV2RvamtMR05GR3A3aHFKUERWUkQzMzR0OFRBSTBsd2Zv?=
 =?utf-8?B?a3VESWxNeFE2T250TkJLUWx3TWhxaUIyK3Z6OW4vcFExakp4Qlh1OFVHZ1JO?=
 =?utf-8?B?WG9oZDJtc2UvbWwrNzhqeVZIRE9VVW9YTXRkTGJvV0l6cTZxTFUybEVlOGUx?=
 =?utf-8?B?Sjd6engzKzhVZmZTS2ppa3JTNW9RVDJaeWM2T24wWFB4SVI2eXlUc2wzV3F4?=
 =?utf-8?B?YjRpZHdLckRsRDhmandCekFuZGNYNXNLMTZ5a1JDU0RleUJqR0lBLytiYm5M?=
 =?utf-8?B?UmtQOE8rN2tsejRkUXdaUkc0czg1RjFyMHVvU1lQcDE1Umt6R0RENktPQUFr?=
 =?utf-8?B?Ry96MmhxV1dncGl1S1FKcXVneklTMFQxbGhZQ0xJUXZYRDBMNm9MTW14NGl5?=
 =?utf-8?B?NHlNNEE5RzZCQmpRaVZFaGRjVDZVUmQ4bjJhdEplMGRUNEVOaGlVTGo5TWZM?=
 =?utf-8?Q?Or54=3D?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 50f7a913-2bad-45d3-c4a0-08d99fef6756
X-MS-Exchange-CrossTenant-AuthSource: AM7PR04MB6821.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Nov 2021 00:01:23.0401
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vKLoGQtFm4WiEgIpYwsxdySEv4n/dodv+r1ur6/yFiR6iktXQZyHT6Kpxliy9FBK
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR0402MB3783
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2021/11/5 07:48, Linus Torvalds wrote:
> On Thu, Nov 4, 2021 at 4:37 PM Linus Torvalds
> <torvalds@linux-foundation.org> 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
>