fs/ext: Support RO_COMPAT_EXTRA_ISIZE #446

2026-05-27T00:05:02Z

Matou1306 commented

2026-05-27 00:05:02 +00:00

This commit provides support for 0x0040 RO_COMPAT_EXTRA_ISIZE.
Umka reading t072 and writing t074 tests are done and passing.

This commit provides support for 0x0040 RO_COMPAT_EXTRA_ISIZE. Umka reading t072 and writing t074 tests are done and passing.

Matou1306 changed title from ~~WIP: Support RO_COMPAT_EXTRA_ISIZE~~ to WIP: fs/ext: Support RO_COMPAT_EXTRA_ISIZE

2026-05-27 00:05:30 +00:00

dunkaist requested changes

2026-05-27 15:28:19 +00:00

Dismissed

kernel/trunk/fs/ext.inc Outdated

						
				@@ -14,2 +14,4 @@

				;   out:

				; eax, ebx = return values for sysfunc 70

				UNIXTIME_TO_KOS_OFFSET = (365*31+8)*24*60*60  ; 01.01.1970 to 01.01.2001

dunkaist commented

2026-05-27 14:37:55 +00:00

This constant will be useful for several filesystem's, e.g. XFS. It is a good idea to move it to a common place like fs/fs_lfn.inc or fs/fs_common.inc

dunkaist marked this conversation as resolved

kernel/trunk/fs/ext.inc Outdated

						
				@@ -157,0 +178,4 @@

				MOUNT_POLICY_NOATIME = 0

				MOUNT_POLICY_ATIME = 1

				MOUNT_POLICY_RELATIME = 2

				MOUNT_POLICY = MOUNT_POLICY_NOATIME ; TODO: add proper mount syscall

dunkaist commented

2026-05-27 15:19:46 +00:00

I believe, this should be a per-partition setting, i.e. a field in the EXTFS structure

Sweetbread marked this conversation as resolved

kernel/trunk/fs/ext.inc Outdated

						
				@@ -161,0 +184,4 @@

				;  1 = sparse superblock

				;  2 = 64-bit file size

				; 40 = extra inode size (WIP)

				READ_ONLY_SUPPORT = 43

dunkaist commented

2026-05-27 14:40:06 +00:00

This must be hex. Same above

dunkaist marked this conversation as resolved

kernel/trunk/fs/ext.inc Outdated

						
				@@ -904,2 +930,2 @@

				        add     eax, 978307200

				        mov     [edi+INODE.inodeModified], eax

				        lea     eax, [edi+INODE.cTime]

				        mov     edx, -1

dunkaist commented

2026-05-27 14:42:09 +00:00

Does this add 0x3fff_ffff nanoseconds? Shouldn't we add 0 ns if the field is missing?

Matou1306 commented

2026-05-28 11:41:45 +00:00

This is just like a flag I made for ext_write_time, the function checks if the extra field is -1 to know if the field doesn't exit to not touch it.

Also I am ignoring nanoseconds (writing it as 0) if that's ok? To get nanoseconds I will have to call an additional system function if I understand correctly.

This is just like a flag I made for ext_write_time, the function checks if the extra field is -1 to know if the field doesn't exit to not touch it. Also I am ignoring nanoseconds (writing it as 0) if that's ok? To get nanoseconds I will have to call an additional system function if I understand correctly.

dunkaist commented

2026-05-29 13:16:27 +00:00

Also I am ignoring nanoseconds (writing it as 0) if that's ok?

It is ok to write just seconds. I doubt any other filesystem in KolibriOS writes nanoseconds at the moment.

> Also I am ignoring nanoseconds (writing it as 0) if that's ok? It is ok to write just seconds. I doubt any other filesystem in KolibriOS writes nanoseconds at the moment.

Sweetbread marked this conversation as resolved

dunkaist reviewed

2026-05-29 13:26:19 +00:00

kernel/trunk/fs/ext.inc Outdated

						
				@@ -1986,0 +2099,4 @@

				@@:

				        jmp     ext_read_time

				.has_all_extra:

				        ; fast path: all Extra fields up to crTimeExtra are present

dunkaist commented

2026-05-29 13:26:19 +00:00

Do I understand correctly the inode size can only be a power of two? I.e. either no extra time is present or all of them are present?

Matou1306 commented

2026-05-29 13:35:10 +00:00

inode_size itself is indeed a power of 2, but the real boundary is the i_extra_isize field at offset 128d: this is the one that specifies the number of extra bytes.
This part of the code is just a shortcut if extra_isize is large enough to contain all extra time fields, it'll skip checking 4 times. If not all fields are present then it will go the slow path checking every field.

I don't know why any partition in the world would have one extra time field but not the other since inode size is either 128 or 256+ which will always fit all of them, but there is no constraint on it, that's why I made the long path of checking before each field. Also extra_isize itself can be different across different inodes, which is why I made the check here instead of global.

inode_size itself is indeed a power of 2, but the real boundary is the i_extra_isize field at offset 128d: this is the one that specifies the number of extra bytes. This part of the code is just a shortcut if extra_isize is large enough to contain all extra time fields, it'll skip checking 4 times. If not all fields are present then it will go the slow path checking every field. I don't know why any partition in the world would have one extra time field but not the other since inode size is either 128 or 256+ which will always fit all of them, but there is no constraint on it, that's why I made the long path of checking before each field. Also extra_isize itself can be different across different inodes, which is why I made the check here instead of global.

👍 1

dunkaist marked this conversation as resolved

Matou1306 force-pushed ext-improvements from 711708ad49 to 80163f0cbe

2026-06-02 22:53:35 +00:00

Compare

Matou1306 force-pushed ext-improvements from cffb9f6f24 to df264189ad

2026-06-02 23:10:26 +00:00

Compare

Matou1306 requested review from Doczom 2026-06-06 23:09:45 +00:00

Matou1306 requested review from Burer 2026-06-06 23:10:05 +00:00

Matou1306 requested review from hidnplayr 2026-06-06 23:10:06 +00:00

Matou1306 commented

2026-06-06 23:11:38 +00:00

I think this should be ready now, it passes umka tests too.
I will clean up and rebase the commits if no more edits are required

I think this should be ready now, it passes umka tests too. I will clean up and rebase the commits if no more edits are required

👀 1

Matou1306 marked the pull request as ready for review 2026-06-06 23:11:50 +00:00

dunkaist approved these changes

2026-06-13 18:47:47 +00:00

Dismissed

dunkaist commented

2026-06-13 18:51:21 +00:00

The code looks good but this PR has 21 commits including other people's commits. Rebase and force push the branch and it should be ok to merge to main then.

👍 1

Matou1306 force-pushed ext-improvements from 9c6a69ed8e to dcd7b68b57

2026-06-13 22:09:13 +00:00

Compare

Matou1306 commented

2026-06-13 22:10:42 +00:00

@dunkaist Done

Doczom approved these changes

2026-06-15 12:14:38 +00:00

Dismissed

Burer requested changes

2026-06-15 14:51:49 +00:00

Dismissed

Burer left a comment

@Matou1306

Please, check this points.
Some of them could be fake alerts, but I believe most is real problems.

🔴 readInode: i_extra_isize is copied unclamped, so an on-disk value >32 overflows the inode struct and makes 32 - extra_isize underflow into a huge rep stosb which leads to kernel memory corruption from untrusted metadata.

🟠 unlinkInode: updates mTime in the buffer but never calls writeInode, so the directory's mtime change is lost (unlike linkInode, which does write it).

🟠 ext_ReadFolder: calls update_aTime on every sub-entry inode, updating each listed file's atime (a directory listing shouldn't touch every file's atime) and causing one inode write per entry.

🟠 MOUNT_POLICY: the db MOUNT_POLICY_NOATIME struct initializer has no effect on the dynamically-allocated partition struct - it only works because NOATIME==0 and the struct is zero-allocated.

🟠 symlink_depth: narrowing it to db while leaving three dword-width accesses (init/dec/cmp) makes those writes spill into the adjacent MOUNT_POLICY byte and past the struct - fix by keeping it dd or changing the three accesses to byte-width.

@Matou1306 Please, check this points. Some of them could be fake alerts, but I believe most is real problems. 🔴 readInode: `i_extra_isize` is copied unclamped, so an on-disk value >32 overflows the inode struct and makes `32 - extra_isize` underflow into a huge `rep stosb` which leads to kernel memory corruption from untrusted metadata. 🟠 unlinkInode: updates `mTime` in the buffer but never calls `writeInode`, so the directory's mtime change is lost (unlike `linkInode`, which does write it). 🟠 ext_ReadFolder: calls `update_aTime` on every sub-entry inode, updating each listed file's atime (a directory listing shouldn't touch every file's atime) and causing one inode write per entry. 🟠 MOUNT_POLICY: the `db MOUNT_POLICY_NOATIME` struct initializer has no effect on the dynamically-allocated partition struct - it only works because NOATIME==0 and the struct is zero-allocated. 🟠 symlink_depth: narrowing it to `db` while leaving three dword-width accesses (init/dec/cmp) makes those writes spill into the adjacent `MOUNT_POLICY` byte and past the struct - fix by keeping it `dd` or changing the three accesses to byte-width.

👍 1

Matou1306 commented

2026-06-18 18:23:47 +00:00

@Burer I implemented your suggestions thanks for that, they pass umka tests.
However, while testing this out on qemu, I found out there is another bug that corrupts everything when I create a new file. I am still uncovering the issue but it corrupts creating files (sometimes creates folders instead, sometimes gives them random file sizes).
The tests were not sufficient I guess... I will fix these issues asap and make sure to also test interactively on qemu in the future.

@Burer I implemented your suggestions thanks for that, they pass umka tests. However, while testing this out on qemu, I found out there is another bug that corrupts everything when I create a new file. I am still uncovering the issue but it corrupts creating files (sometimes creates folders instead, sometimes gives them random file sizes). The tests were not sufficient I guess... I will fix these issues asap and make sure to also test interactively on qemu in the future.

👍 1

Matou1306 commented

2026-06-18 23:31:56 +00:00

@Burer @dunkaist I pushed a commit that should fix the bug, I just forgot to reload ebx after calling ext_write_time. I updated it so that the proc uses edi esi ebx to push/pop them instead. I created a new t076 for umka which tests ext create file, which used to fail on the code one commit ago but is now succeeding. I also tried testing on qemu, created a file, edited a file, and it works normally now. Please take a look.

👀 1

dunkaist force-pushed ext-improvements from b39dce6b55 to 21a56f5862

2026-06-20 17:24:48 +00:00

Compare

dunkaist approved these changes

2026-06-20 18:17:54 +00:00

Dismissed

Burer commented

2026-06-21 11:23:50 +00:00

Hi, @Matou1306

Great work!

Now I can see only a few small problems, check them also, please:
🟠 update_aTime: routes atime updates through writeInode, which always stamps cTime=now, so an atime refresh on read also bumps ctime - should be fixed before enabling atime by writing only the atime field instead of the whole inode.
🟡 ext_CreateFile / ext_CreateFolder: zeroes the inode and set aTime/mTime but never set crTime or i_extra_isize, so files created on an extra_isize volume get no creation time and no sub-second fields (extra_isize stays 0).
🟡 ext_write_time: writes only the epoch bits (and edx, 3) into *_time_extra, zeroing the nanosecond field on every timestamp update, so any sub-second precision set by Linux is dropped.

Hi, @Matou1306 Great work! Now I can see only a few small problems, check them also, please: 🟠 `update_aTime`: routes atime updates through `writeInode`, which always stamps `cTime=now`, so an atime refresh on read also bumps ctime - should be fixed before enabling atime by writing only the atime field instead of the whole inode. 🟡 `ext_CreateFile` / `ext_CreateFolder`: zeroes the inode and set `aTime`/`mTime` but never set `crTime` or `i_extra_isize`, so files created on an `extra_isize` volume get no creation time and no sub-second fields (`extra_isize` stays 0). 🟡 `ext_write_time`: writes only the epoch bits (`and edx, 3`) into `*_time_extra`, zeroing the nanosecond field on every timestamp update, so any sub-second precision set by Linux is dropped.

Matou1306 commented

2026-06-22 13:18:58 +00:00

@Burer Thanks, I will work on them.
For the first one when I updated the code I stuck with the previous structure which updated time fields in write_Inode. Linux does it very differently by having another process write it asynchronously, for KolibriOS I think I can just input a flag to the function that decides whether to update cTime or not.

Regarding sub-second fields we are only writing the seconds, saving the old nanosecond field would be meaningless I guess because we already updated the timestamp. Say the old time was 13:43:13.02341 then we update it to 21:34:50 saving those 0.02341 would actually be incorrect here (?)

@Burer Thanks, I will work on them. For the first one when I updated the code I stuck with the previous structure which updated time fields in write_Inode. Linux does it very differently by having another process write it asynchronously, for KolibriOS I think I can just input a flag to the function that decides whether to update cTime or not. Regarding sub-second fields we are only writing the seconds, saving the old nanosecond field would be meaningless I guess because we already updated the timestamp. Say the old time was 13:43:13.02341 then we update it to 21:34:50 saving those 0.02341 would actually be incorrect here (?)

Burer commented

2026-06-22 14:56:19 +00:00

@Matou1306

It's up to you and your mentor to decide support nanoseconds or not.
I agree that this is not crucial for KOS and can be skipped.
Just listed all found problems for your information.

@Matou1306 It's up to you and your mentor to decide support nanoseconds or not. I agree that this is not crucial for KOS and can be skipped. Just listed all found problems for your information.

dunkaist commented

2026-06-22 17:39:05 +00:00

It's up to you and your mentor to decide support nanoseconds or not.

I think nanoseconds can be ignored/zeroed for KolibriOS.

> It's up to you and your mentor to decide support nanoseconds or not. I think nanoseconds can be ignored/zeroed for KolibriOS.

Matou1306 force-pushed ext-improvements from 21a56f5862 to b8d304125a

2026-06-23 12:58:16 +00:00

Compare

Matou1306 commented

2026-06-23 13:04:30 +00:00

@Burer I worked on the first 2 points, for the nanoseconds as @dunkaist mentioned we will be ignoring it.

❤️ 1

Doczom approved these changes

2026-06-23 17:44:43 +00:00

Dismissed

Burer commented

2026-06-24 06:57:47 +00:00

@Matou1306

Seems good to me!
But one small latent bug was introduced in latest commits.

writeInode_no_cTime skips the mov edi, ebx that sets up the inode-buffer pointer:

@@:
        push    edi esi ecx ebx eax
        cmp     edx, -1
        je      .ignoreCTime    ; no_cTime path jumps over the next line
        mov     edi, ebx        ; only runs on the normal writeInode path

So when entered via writeInode_no_cTime, edi keeps the caller's leftover value instead of pointing at the inode buffer. The root-inode cache update then reuses it:

        mov     esi, edi        ; esi = garbage in the no_cTime path
        lea     edi, [rootInodeBuffer]
        rep movsb               ; copies garbage into rootInodeBuffer

Writing the root inode through writeInode_no_cTime therefore corrupts the cached root inode.

It is not triggered today, as the only caller is update_aTime, which never writes the root inode (ReadFile rejects directories). But it is a latent trap for any future no_cTime write of the root inode.

Fix should be simple - move mov edi, ebx above the cmp edx, -1 so edi is always initialized, as it was before this commit.

@Matou1306 Seems good to me! But one small latent bug was introduced in latest commits. `writeInode_no_cTime` skips the `mov edi, ebx` that sets up the inode-buffer pointer: ```asm @@: push edi esi ecx ebx eax cmp edx, -1 je .ignoreCTime ; no_cTime path jumps over the next line mov edi, ebx ; only runs on the normal writeInode path ``` So when entered via `writeInode_no_cTime`, edi keeps the caller's leftover value instead of pointing at the inode buffer. The root-inode cache update then reuses it: ```asm mov esi, edi ; esi = garbage in the no_cTime path lea edi, [rootInodeBuffer] rep movsb ; copies garbage into rootInodeBuffer ``` Writing the root inode through `writeInode_no_cTime` therefore corrupts the cached root inode. It is not triggered today, as the only caller is `update_aTime`, which never writes the root inode (ReadFile rejects directories). But it is a latent trap for any future `no_cTime` write of the root inode. Fix should be simple - move `mov edi, ebx` above the `cmp edx, -1` so `edi` is always initialized, as it was before this commit.

Matou1306 force-pushed ext-improvements from b8d304125a to 7788c14683

2026-06-24 07:29:42 +00:00

Compare

Matou1306 commented

2026-06-24 07:34:32 +00:00

@Burer Thanks for catching that!
When I looked at it the first time I didn't think of it much and thought it was some preparation for fsGetTime call 😐.

@Burer Thanks for catching that! When I looked at it the first time I didn't think of it much and thought it was some preparation for `fsGetTime` call 😐.

❤️ 1

Burer approved these changes

2026-06-24 07:57:37 +00:00

Burer commented

2026-06-24 07:57:58 +00:00

@Matou1306

Thanks you for accurate work!

@Matou1306 Thanks you for accurate work!

❤️ 1

Burer force-pushed ext-improvements from 7788c14683 to fb63b9c5ad

2026-06-24 07:58:47 +00:00

Compare