Page MenuHomeFreeBSD

[tmpfs]: Fix misc issues reproduced by pjdfstest
ClosedPublic

Authored by fsu on Jan 14 2023, 12:21 PM.
Tags
None
Referenced Files
F82375290: D38051.id.diff
Sat, Apr 27, 10:51 PM
Unknown Object (File)
Fri, Apr 26, 8:50 AM
Unknown Object (File)
Fri, Apr 26, 8:48 AM
Unknown Object (File)
Fri, Apr 26, 8:48 AM
Unknown Object (File)
Fri, Apr 26, 1:40 AM
Unknown Object (File)
Jan 15 2024, 12:18 AM
Unknown Object (File)
Dec 20 2023, 4:54 AM
Unknown Object (File)
Dec 3 2023, 3:11 AM
Subscribers

Details

Summary

pjdfstest/tests/rename/09.t:
Fix rename when renamed directory not owned by user, but when user owns the sticky parent directory.

pjdfstest/tests/rename/rename/19.t:
The rename call with args like "./dir0/dir1/.." "./dir2" will cause MPASS failure.
The tmpfs_dir_lookup() does not accept names like '.' and '..' for lookup.
Move the '.' and '..' entry check before tmpfs_dir_lookup() call.

pjdfstest/tests/rename/23.t
This test creates two files like file0 and file1, then creates link to file1 and checks ctime on it.
Then renames file0 to file1. Then checks ctime on link again. It is expected, that second ctime will be higher then
first ctime, because rename happen. Add ctime updating for directory entry, which will be deleted on rename.

Test Plan

All pjdfstest testcases are passed successfully now.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

fsu requested review of this revision.Jan 14 2023, 12:21 PM

Add links to testcases to source code.

fs/tmpfs/tmpfs_vnops.c
1031 ↗(On Diff #115125)

pjdfstest/tests/rename/rename/19.t

1124 ↗(On Diff #115125)

pjdfstest/tests/rename/09.t

1248 ↗(On Diff #115125)

pjdfstest/tests/rename/23.t

fs/tmpfs/tmpfs_vnops.c
1039 ↗(On Diff #115125)

This part is fine

1132 ↗(On Diff #115125)
1135 ↗(On Diff #115125)

Look at ufs_rename() how this case is handled. I believe it is ufs_vnops.c:1619

(I suspect that in your version, regardless of ufs approach, it should be tdp and not fdp)

1250 ↗(On Diff #115125)

This one is also fine, I suspect that tmpfs_update() is excessive.

fs/tmpfs/tmpfs_vnops.c
1135 ↗(On Diff #115125)

The error for this case with sticky bit is handled by VOP_ACCESS, which will return EACCESS in ufs_vnops.c:1485

1250 ↗(On Diff #115125)

Yep, the test will pass without tmpfs_update(), because ctime will be updated on the next stat syscall:
tmpfs_itimes() at tmpfs_itimes+0x104/frame 0xfffffe00e8c69b90
tmpfs_stat() at tmpfs_stat+0x203/frame 0xfffffe00e8c69bc0
kern_statat() at kern_statat+0x1aa/frame 0xfffffe00e8c69d00
sys_fstatat() at sys_fstatat+0x2f/frame 0xfffffe00e8c69e00
amd64_syscall() at amd64_syscall+0x12e/frame 0xfffffe00e8c69f30
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe00e8c69f30

But is it correct to have ctime updated not under rename syscall?
The ctime could be incorrect in case if stat syscall will be delayed.

I suggest you to commit two fixes that do not cause questions. These should be two separate commits.

Then we can continue with the sticky bit.

fs/tmpfs/tmpfs_vnops.c
1135 ↗(On Diff #115125)

No, 1485 is about different thing, not related to sticky bit. You must be able to write to the directory to move it to different parent.

1250 ↗(On Diff #115125)

Do you mean that ctime would be too new when stat/getattr occurs?

It seems that UFS handles this correctly, and tmpfs indeed does not, because UFS calls UFS_UPDATE() in inactive VOP. This is missed in tmpfs inactivation, which only updates mtime.

Anyway, calling tmpfs_update() there is a small thing.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 29 2023, 8:21 AM
This revision was automatically updated to reflect the committed changes.
fs/tmpfs/tmpfs_vnops.c
1135 ↗(On Diff #115125)

I cannot understand is it correct or not for now, but the error for this pjdfstest test-case will be returned from here in case of ufs and
the test will pass. Let's open review specially for this case: [https://reviews.freebsd.org/D38245]

1250 ↗(On Diff #115125)

Yep, let's imagine the situation, that between rename and stat calls there was 10 seconds interval. User will see ctime 10 seconds newer, than actual ctime on rename.

fs/tmpfs/tmpfs_vnops.c
1250 ↗(On Diff #115125)

I mean that tmpfs inactivation should do the inode update (as well).