| Message ID | 20230712150251.163790-1-jlayton@kernel.org |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ext4: fix decoding of raw_inode timestamps | expand |
On Wed, 12 Jul 2023 11:02:49 -0400, Jeff Layton wrote: > When we covert a timestamp from raw disk format, we need to consider it > to be signed, as the value may represent a date earlier than 1970. This > fixes generic/258 on ext4. > > Applied to the vfs.ctime branch of the vfs/vfs.git tree. Patches in the vfs.ctime branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.ctime [1/1 FOLDED] ext4: convert to ctime accessor functions https://git.kernel.org/vfs/vfs/c/f65cb009d449
On Wed, Jul 12, 2023 at 11:02:49AM -0400, Jeff Layton wrote: > When we covert a timestamp from raw disk format, we need to consider it > to be signed, as the value may represent a date earlier than 1970. This > fixes generic/258 on ext4. > > Cc: Jan Kara <jack@suse.cz> > Fixes: f2ddb05870fb ("ext4: convert to ctime accessor functions") > Signed-off-by: Jeff Layton <jlayton@kernel.org> Acked-by: Theodore Ts'o <tytso@mit.edu> Thanks for the fix! It had been on my list to checking to see if the ext4 kunit tests would pass, since Jan had mentioned that he had done the work to make sure the ext4 kunit test would compile, but he hadn't gotten around to try run the kunit test. Unfortunately, I hadn't gotten to it. I *think* the ext4 kunit tests should have caught this as well; out of curiosity, have you tried running the ext4 kunit tests either before or after this patch? If so, what were your findings? Cheers, - Ted
On Wed, 2023-07-12 at 13:52 -0400, Theodore Ts'o wrote: > On Wed, Jul 12, 2023 at 11:02:49AM -0400, Jeff Layton wrote: > > When we covert a timestamp from raw disk format, we need to consider it > > to be signed, as the value may represent a date earlier than 1970. This > > fixes generic/258 on ext4. > > > > Cc: Jan Kara <jack@suse.cz> > > Fixes: f2ddb05870fb ("ext4: convert to ctime accessor functions") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > Acked-by: Theodore Ts'o <tytso@mit.edu> > > Thanks for the fix! > > It had been on my list to checking to see if the ext4 kunit tests > would pass, since Jan had mentioned that he had done the work to make > sure the ext4 kunit test would compile, but he hadn't gotten around to > try run the kunit test. Unfortunately, I hadn't gotten to it. > > I *think* the ext4 kunit tests should have caught this as well; out of > curiosity, have you tried running the ext4 kunit tests either before > or after this patch? If so, what were your findings? > > Cheers, > > - Ted No, I haven't. I'm running fstests on it now. Is there a quickstart for running those tests?
On Wed, Jul 12, 2023 at 02:09:59PM -0400, Jeff Layton wrote: > > No, I haven't. I'm running fstests on it now. Is there a quickstart for > running those tests? At the top level kernel sources: ./tools/testing/kunit/kunit.py run --kunitconfig ./fs/ext4/.kunitconfig You should get: [17:23:09] Starting KUnit Kernel (1/1)... [17:23:09] ============================================================ [17:23:09] =============== ext4_inode_test (1 subtest) ================ [17:23:09] ============= inode_test_xtimestamp_decoding ============== [17:23:09] [PASSED] 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits [17:23:09] [PASSED] 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits [17:23:09] [PASSED] 1970-01-01 Lower bound of 32bit >=0 timestamp, no extra bits [17:23:09] [PASSED] 2038-01-19 Upper bound of 32bit >=0 timestamp, no extra bits [17:23:09] [PASSED] 2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on [17:23:09] [PASSED] 2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on [17:23:09] [PASSED] 2106-02-07 Lower bound of 32bit >=0 timestamp, lo extra sec bit on [17:23:09] [PASSED] 2174-02-25 Upper bound of 32bit >=0 timestamp, lo extra sec bit on [17:23:09] [PASSED] 2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on [17:23:09] [PASSED] 2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on [17:23:09] [PASSED] 2242-03-16 Lower bound of 32bit >=0 timestamp, hi extra sec bit on [17:23:09] [PASSED] 2310-04-04 Upper bound of 32bit >=0 timestamp, hi extra sec bit on [17:23:09] [PASSED] 2310-04-04 Upper bound of 32bit>=0 timestamp, hi extra sec bit 1. 1 ns [17:23:09] [PASSED] 2378-04-22 Lower bound of 32bit>= timestamp. Extra sec bits 1. Max ns [17:23:09] [PASSED] 2378-04-22 Lower bound of 32bit >=0 timestamp. All extra sec bits on [17:23:09] [PASSED] 2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on [17:23:09] ========= [PASSED] inode_test_xtimestamp_decoding ========== [17:23:09] ================= [PASSED] ext4_inode_test ================= [17:23:09] ============================================================ [17:23:09] Testing complete. Ran 16 tests: passed: 16 [17:23:09] Elapsed time: 1.943s total, 0.001s configuring, 1.777s building, 0.123s running - Ted
On Wed, 2023-07-12 at 17:25 -0400, Theodore Ts'o wrote: > On Wed, Jul 12, 2023 at 02:09:59PM -0400, Jeff Layton wrote: > > > > No, I haven't. I'm running fstests on it now. Is there a quickstart for > > running those tests? > > At the top level kernel sources: > > ./tools/testing/kunit/kunit.py run --kunitconfig ./fs/ext4/.kunitconfig > > You should get: > > [17:23:09] Starting KUnit Kernel (1/1)... > [17:23:09] ============================================================ > [17:23:09] =============== ext4_inode_test (1 subtest) ================ > [17:23:09] ============= inode_test_xtimestamp_decoding ============== > [17:23:09] [PASSED] 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits > [17:23:09] [PASSED] 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits > [17:23:09] [PASSED] 1970-01-01 Lower bound of 32bit >=0 timestamp, no extra bits > [17:23:09] [PASSED] 2038-01-19 Upper bound of 32bit >=0 timestamp, no extra bits > [17:23:09] [PASSED] 2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on > [17:23:09] [PASSED] 2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on > [17:23:09] [PASSED] 2106-02-07 Lower bound of 32bit >=0 timestamp, lo extra sec bit on > [17:23:09] [PASSED] 2174-02-25 Upper bound of 32bit >=0 timestamp, lo extra sec bit on > [17:23:09] [PASSED] 2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on > [17:23:09] [PASSED] 2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on > [17:23:09] [PASSED] 2242-03-16 Lower bound of 32bit >=0 timestamp, hi extra sec bit on > [17:23:09] [PASSED] 2310-04-04 Upper bound of 32bit >=0 timestamp, hi extra sec bit on > [17:23:09] [PASSED] 2310-04-04 Upper bound of 32bit>=0 timestamp, hi extra sec bit 1. 1 ns > [17:23:09] [PASSED] 2378-04-22 Lower bound of 32bit>= timestamp. Extra sec bits 1. Max ns > [17:23:09] [PASSED] 2378-04-22 Lower bound of 32bit >=0 timestamp. All extra sec bits on > [17:23:09] [PASSED] 2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on > [17:23:09] ========= [PASSED] inode_test_xtimestamp_decoding ========== > [17:23:09] ================= [PASSED] ext4_inode_test ================= > [17:23:09] ============================================================ > [17:23:09] Testing complete. Ran 16 tests: passed: 16 > [17:23:09] Elapsed time: 1.943s total, 0.001s configuring, 1.777s building, 0.123s running > > - Ted Thanks Ted, The above output is what I get with the fix in place. Without this patch, I get: $ make ARCH=um O=.kunit --jobs=16 [06:46:35] Starting KUnit Kernel (1/1)... [06:46:35] ============================================================ [06:46:35] =============== ext4_inode_test (1 subtest) ================ [06:46:35] ============= inode_test_xtimestamp_decoding ============== [06:46:35] # inode_test_xtimestamp_decoding: EXPECTATION FAILED at fs/ext4/inode-test.c:252 [06:46:35] Expected test_param->expected.tv_sec == timestamp.tv_sec, but [06:46:35] test_param->expected.tv_sec == -2147483648 (0xffffffff80000000) [06:46:35] timestamp.tv_sec == 2147483648 (0x80000000) [06:46:35] 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits: msb:1 lower_bound:1 extra_bits: 0 [06:46:35] [FAILED] 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits [06:46:35] # inode_test_xtimestamp_decoding: EXPECTATION FAILED at fs/ext4/inode-test.c:252 [06:46:35] Expected test_param->expected.tv_sec == timestamp.tv_sec, but [06:46:35] test_param->expected.tv_sec == -1 (0xffffffffffffffff) [06:46:35] timestamp.tv_sec == 4294967295 (0xffffffff) [06:46:35] 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits: msb:1 lower_bound:0 extra_bits: 0 [06:46:35] [FAILED] 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits [06:46:35] [FAILED] 1970-01-01 Lower bound of 32bit >=0 timestamp, no extra bits [06:46:35] [FAILED] 2038-01-19 Upper bound of 32bit >=0 timestamp, no extra bits [06:46:35] # inode_test_xtimestamp_decoding: EXPECTATION FAILED at fs/ext4/inode-test.c:252 [06:46:35] Expected test_param->expected.tv_sec == timestamp.tv_sec, but [06:46:35] test_param->expected.tv_sec == 2147483648 (0x80000000) [06:46:35] timestamp.tv_sec == 6442450944 (0x180000000) [06:46:35] 2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on: msb:1 lower_bound:1 extra_bits: 1 [06:46:35] [FAILED] 2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on [06:46:35] # inode_test_xtimestamp_decoding: EXPECTATION FAILED at fs/ext4/inode-test.c:252 [06:46:35] Expected test_param->expected.tv_sec == timestamp.tv_sec, but [06:46:35] test_param->expected.tv_sec == 4294967295 (0xffffffff) [06:46:35] timestamp.tv_sec == 8589934591 (0x1ffffffff) [06:46:35] 2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on: msb:1 lower_bound:0 extra_bits: 1 [06:46:35] [FAILED] 2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on [06:46:35] [FAILED] 2106-02-07 Lower bound of 32bit >=0 timestamp, lo extra sec bit on [06:46:35] [FAILED] 2174-02-25 Upper bound of 32bit >=0 timestamp, lo extra sec bit on [06:46:35] # inode_test_xtimestamp_decoding: EXPECTATION FAILED at fs/ext4/inode-test.c:252 [06:46:35] Expected test_param->expected.tv_sec == timestamp.tv_sec, but [06:46:35] test_param->expected.tv_sec == 6442450944 (0x180000000) [06:46:35] timestamp.tv_sec == 10737418240 (0x280000000) [06:46:35] 2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on: msb:1 lower_bound:1 extra_bits: 2 [06:46:35] [FAILED] 2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on [06:46:35] # inode_test_xtimestamp_decoding: EXPECTATION FAILED at fs/ext4/inode-test.c:252 [06:46:35] Expected test_param->expected.tv_sec == timestamp.tv_sec, but [06:46:35] test_param->expected.tv_sec == 8589934591 (0x1ffffffff) [06:46:35] timestamp.tv_sec == 12884901887 (0x2ffffffff) [06:46:35] 2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on: msb:1 lower_bound:0 extra_bits: 2 [06:46:35] [FAILED] 2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on [06:46:35] [FAILED] 2242-03-16 Lower bound of 32bit >=0 timestamp, hi extra sec bit on [06:46:35] [FAILED] 2310-04-04 Upper bound of 32bit >=0 timestamp, hi extra sec bit on [06:46:35] [FAILED] 2310-04-04 Upper bound of 32bit>=0 timestamp, hi extra sec bit 1. 1 ns [06:46:35] [FAILED] 2378-04-22 Lower bound of 32bit>= timestamp. Extra sec bits 1. Max ns [06:46:35] [FAILED] 2378-04-22 Lower bound of 32bit >=0 timestamp. All extra sec bits on [06:46:35] [FAILED] 2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on [06:46:35] # inode_test_xtimestamp_decoding: pass:0 fail:16 skip:0 total:16 [06:46:35] ========= [FAILED] inode_test_xtimestamp_decoding ========== [06:46:35] # Totals: pass:0 fail:16 skip:0 total:16 [06:46:35] ================= [FAILED] ext4_inode_test ================= [06:46:35] ============================================================ [06:46:35] Testing complete. Ran 16 tests: failed: 16 [06:46:35] Elapsed time: 14.549s total, 0.002s configuring, 14.229s building, 0.275s running Cheers,
On Thu, Jul 13, 2023 at 06:48:04AM -0400, Jeff Layton wrote: > > The above output is what I get with the fix in place. Without this > patch, I get: ... Thanks!! It's good to know the _one_ kunit test we have is capable of detecting this. We have a patch series lined up to add our *second* unit test (for the block allocator) for the next merge window, and while our unit test coverage is still quite small, it's nice to know that it can detect problems --- and much faster than running xfstests. :-} - Ted
On Thu, 2023-07-13 at 09:04 -0400, Theodore Ts'o wrote: > On Thu, Jul 13, 2023 at 06:48:04AM -0400, Jeff Layton wrote: > > > > The above output is what I get with the fix in place. Without this > > patch, I get: ... > > Thanks!! It's good to know the _one_ kunit test we have is capable of > detecting this. We have a patch series lined up to add our *second* > unit test (for the block allocator) for the next merge window, and > while our unit test coverage is still quite small, it's nice to know > that it can detect problems --- and much faster than running xfstests. :-} > Yeah, it's pretty quick! I need to consider adding some tests for some other areas that are difficult to view outside the kernel (the errseq_t infrastructure comes to mind).
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index d63543187359..2af347669db7 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -877,7 +877,7 @@ static inline __le32 ext4_encode_extra_time(struct timespec64 ts) static inline struct timespec64 ext4_decode_extra_time(__le32 base, __le32 extra) { - struct timespec64 ts = { .tv_sec = le32_to_cpu(base) }; + struct timespec64 ts = { .tv_sec = (signed)le32_to_cpu(base) }; if (unlikely(extra & cpu_to_le32(EXT4_EPOCH_MASK))) ts.tv_sec += (u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32;
When we covert a timestamp from raw disk format, we need to consider it to be signed, as the value may represent a date earlier than 1970. This fixes generic/258 on ext4. Cc: Jan Kara <jack@suse.cz> Fixes: f2ddb05870fb ("ext4: convert to ctime accessor functions") Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ext4/ext4.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) It might be best to just squash this fix in with the ext4 conversion in the vfs tree.