[MDEV-11759] Encryption code in MariaDB 10.1/10.2 causes compatibility problems Created: 2017-01-10 Updated: 2019-02-09 Resolved: 2017-03-13 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Storage Engine - InnoDB |
| Affects Version/s: | 10.1, 10.2 |
| Fix Version/s: | 10.1.22, 10.2.5 |
| Type: | Bug | Priority: | Critical |
| Reporter: | Michael Graf | Assignee: | Jan Lindström (Inactive) |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | 10.2-rc | ||
| Environment: |
Windows Server 2012 R2 Standard |
||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||||||||||
| Sprint: | 10.2.4-1, 10.2.4-2, 10.1.22 | ||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
MariaDB server 10.2.3 stuck after our application did some inserts/updates/deletes/selects. To avoid the crash from |
| Comments |
| Comment by Marko Mäkelä [ 2017-01-23 ] | ||||||||||||||||||||||||
|
In the error log, I spotted the following:
This is the wrong course of action. The InnoDB system tablespace contains essential data (enumerated in MDEV-11633) and we cannot disable access to it. In the error log, I also recovered a page dump. It looks like an unencrypted B-tree page. If I am guessing correctly, it is the clustered index leaf page of some table whose PRIMARY KEY consists of three 32-bit integers. But the DB_TRX_ID of most (if not all) records would look suspiciously small, only 0x546. Either these records were inserted soon after the database was originally initialized, or we wrongly reset the TRX_ID sequence when access to the system tablespace was disabled. The records look a little strange for me, because some records are longer, some shorter, and some contain text strings of timestamps delimited by # characters. The linked list of records on the page looks correct to me. Finally, I got curious and wanted to run innochecksum on the 16k page dump that I extracted from the hexdump in the error log. For some reason, innochecksum is not reporting the page as corrupted, but single-stepping in gdb revealed this problem:
That is, innochecksum (and I guess also MariaDB Server) wrongly identifies the page as an encrypted one. Luckily for us, the first check fails (in a worse case, we could mistake a corrupted unencrypted page for a valid encrypted one), and then we fall back to the unencrypted check. But, that check is bogus for this page:
Because the mach_read_from_4() happened to return 30 for this page, buf_page_is_corrupted() will wrongly assume that the page is encrypted, and ultimately claim that the page is valid, even though we just determined that it cannot be a valid encrypted page. How did we get the value 30 into the field FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION? Finally, the hang in the error log is triggered by buf_wait_for_read() for this code:
Apparently the hang occurs either at the start of query execution when ha_innobase::records_in_range() is being invoked, or during CHECK TABLE. The function buf_wait_for_read() is identical with MySQL 5.7 (including the typo in the comment). This area of the code was last changed several years ago (Oracle Bug#16249481), so we might want to look for the culprit elsewhere in the MariaDB Server 10.2 code base. One simple explanation could be that when the modified InnoDB in MariaDB 10.2 noticed page corruption, it forgot to release the block->lock, setting a ‘trap’ for any subsequent thread that might want to access the block. | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-01-23 ] | ||||||||||||||||||||||||
|
I think that we must fix several independent issues:
Note that MySQL 5.7 contains this change (to prepare for the repurposing of the field as FIL_RTREE_SPLIT_SEQ_NUM), but it does not retroactively clear the field to 0 on old files:
| ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-01-23 ] | ||||||||||||||||||||||||
|
Note: The value 30 would be an unlikely small value of FIL_PAGE_FLUSH_LSN. A more likely explanation could be that the system tablespace file was originally created with an old version of MySQL where InnoDB did not bother to initialize unused data fields, writing garbage contents. | ||||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-02-02 ] | ||||||||||||||||||||||||
|
bb-10.1- | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-02-02 ] | ||||||||||||||||||||||||
|
I think that more work is needed to ensure that the encryption code is as compatible with old files as possible. (Old files may contain uninitialized garbage in data fields that have been repurposed for encryption.) Instead of trusting the page contents before checksum validation, we should check fil_space_t if the pages are supposed to be encrypted. For encrypted pages, we can assume that FIL_PAGE_TYPE is valid. For unencrypted pages, we cannot assume so, because MySQL 5.1 and earlier did not always initialize FIL_PAGE_TYPE. | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-02-06 ] | ||||||||||||||||||||||||
|
Another data set (page 0 of the system tablespace, and page 8192 of the system tablespace, in the first page of a second file of the system tablespace) is attached in the duplicate issue | ||||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-02-06 ] | ||||||||||||||||||||||||
|
commit ddf2fac73381b84114d31c178d9207afc27bfa4d compatibility problems Pages that are encrypted contain post encryption checksum on Pages that are page compressed do not contain any checksum, buf0buf.cc: buf_page_is_corrupted() mofified so that buf0buf.h, buf_block_init(), buf_page_init_low(): buf_page_get_gen(): use new buf_page_check_corrupt() function buf_page_check_corrupt(): If page was not yet decrypted If page is detected as corrupted and it is not encrypted buf_page_io_complete(): use new buf_page_check_corrupt() buf_page_decrypt_after_read(): Verify post encryption fil0crypt.cc: fil_encrypt_buf() verify post encryption fil_space_verify_crypt_checksum(): rewrite to use fil0fil.ic: Add missed page type encrypted and page Note that this change does not yet fix innochecksum tool, Fix test failures caused by buf page corruption injection. | ||||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-02-07 ] | ||||||||||||||||||||||||
|
Addressing review comments: | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-02-20 ] | ||||||||||||||||||||||||
|
I noticed some problems when running the test innodb.innodb_bug14147491, which is intentionally corrupting a data page.
Please do the error handling at the lowest possible level, and avoid accessing the already freed buffer block. | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-02-23 ] | ||||||||||||||||||||||||
|
In this encryption.innodb_onlinealter_encryption failure we are attempting to re-read a page of a non-existing tablespace:
It seems that fil_crypt_get_page_throttle_func() should use fil_space_acquire_silent() and fil_space_release(). But maybe we should also revise the reread logic to prevent such a futile operation. A missing tablespace cannot reappear, so there is no point to retry. | ||||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-02-27 ] | ||||||||||||||||||||||||
|
https://github.com/MariaDB/server/commit/3412e6a73b45a83ba45368be22bc162bbf39ff2b | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-02-27 ] | ||||||||||||||||||||||||
|
I think that the patch needs some more work, and it also seems to depend on some changes that are being done in | ||||||||||||||||||||||||
| Comment by Jan Lindström (Inactive) [ 2017-03-01 ] | ||||||||||||||||||||||||
|
https://github.com/MariaDB/server/commit/dc166632ca30071d83c4a1c14de9b322c9510f6e | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-03-02 ] | ||||||||||||||||||||||||
|
I think that we need some more testing to cover the error cases. | ||||||||||||||||||||||||
| Comment by Marko Mäkelä [ 2017-03-14 ] | ||||||||||||||||||||||||
|
The comments after 2017-02-07 will be addressed as part of |