Hey J. R. Okajima, Thanks for the quick response!
On Wed, Oct 6, 2021 at 9:40 PM <hooanon...@gmail.com> wrote: > > Hi Mauricio, > > Mauricio Faria de Oliveira: > > The key change is very simple (set `path.mnt` in vfsub_lookup_one_len()) > > but its caller chain is large enough (reviewed/modified ~30 functions). > > Fortunately most of them already had `path.mnt` set or easy to obtain. > > Thanx for the report. You made a good analysis again. > But I am afraid I cannot fully understand the problem yet. Let me make > sure a few things. > > - What is the "real" trigger? > I mean the "root cause of the problem." The root cause of the problem is that vfsub_lookup_one_len[_unlocked]() set `struct path.mnt`to NULL for vfs_getattr(), and when VFS accesses any field in there (e.g., `struct vfsmount.mnt_flags`), it hits a kernel oops. > If your branches were tmpfs+ext4 or something, the problem should not > happen. Simple open(2) should succeed obviously. Is it because of > the writable FUSE branch? > As you might know, the behaviour of vfsub_update_h_iattr() depends > upon whether the branch is FUSE or not. Yes, a fuseblk (not regular fuse) mount is required, since it satisfies the two checks in vfsub_update_h_iattr() (in the `*did =` assignment): 1) !au_test_fs_remote(), as it has FS_REQUIRES_DEV (check with `grep fuse /proc/filesystems`) 2) au_test_fs_refresh_iattr(), as it has a FUSE superblock magic > > - vfs_getattr_nosec() (in AppArmor disabled case) > and > common_perm_cond() (in AppArmor enabled case), > they refer path->mnt since the commits > 549c7297717c3 2021-01-24 fs: make helpers idmap mount aware > 3cee6079f62f4 2021-01-24 apparmor: handle idmapped mounts > Currently I guess these commits are largely related to this problem, > and I don't agree that the bug was born 10 years ago. > Right, I said 'potentially' because I couldn't actually test the details, but the commit mentioned in patch 2 that introduced the `mntflags` access in apparmor that is still hit nowadays, is ~10 years old, > Assuming the writable FUSE branch is close to one of the cause, I will > find the simplest and minimalst FUSE fs and try reproducing the problem > on my side. > ntfs-3g (you mentioned) didn't co-work well on my test environment. > Installing the package (I'm a debian user) complains about EFI brabra > and I gave up ntfs-3g. Right, any fuseblk (not regular fuse) should do the trick as it passes the two checks (as described above); I used ntfs-3g just because it came up first for a search on fuseblk. :) If a VM works for you, that might fit this testing best, as your kernel is expected to hit an Oops if the reproducer works. At least in the VM that I tested this on, it didn't continue working well afterward. > > Anyway I think I should try the old bug report on unix.stackexchange.com > you mentioned, which I didn't know, after finishing this problem. > > About the patch: > Instead of adding a parameter 'struct vfsmount *mnt' to > vfsub_lookup_one_len() family, I'd rather convert the type of parameter > 'parent' into struct path. Ok, cool. Initially I didn't want to change existing things unless you preferred it that way, but now I'm happy to send that as v2. Thanks! > > > J. R. Okajima -- Mauricio Faria de Oliveira
