The panic() will be called under ext2_dirbad() function in case of rw mount.
It cause user confusion, like in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=265951
Replace panic to printf and add error handling logic.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Hmm.. we should keep consistency with UFS .. and that means panic in ext2_dirbad(). My guess is that for this case you shouldn' t call ext2_dirbad().
Comment Actions
While I agree with the sentiment that panicing is a rather extreme measure, it is better to keep consistency between UFS and ext2.
I am still figuring this out .... other than some macros that UFS has added, it does seem we are currently consistent. Does UFS panic and is this something that should be done there as well? Perhaps we should coordinate something with Kirk.
| fs/ext2fs/ext2_lookup.c | ||
|---|---|---|
| 543 ↗ | (On Diff #116980) | Please check ufs/ufs/ufs_lookup.c |
| 806 ↗ | (On Diff #116980) | We shouldn't change this. |
Comment Actions
Per my comment, both ext2 and ufs should no longer panic in ufs_baddir.
| fs/ext2fs/ext2_lookup.c | ||
|---|---|---|
| 806 ↗ | (On Diff #116980) | In reviewing UFS, I believe that the panic should be changed to a printf in both UFS and ext2. At one time it was not plausibly possible to recover when this condition occurred, but today the instance from which recovery was impossible no longer exists. In the case of ufs_dirbad the first part in `if((mp->mnt_flag & MNT_RDONLY) == 0)' should be removed and the else clause should always be done. There is the question of how we got to the point of this error, but the fact that we can recover from it means that we have no need to panic. |
Comment Actions
Ok, in case of ext2fs, we not need printf and leave only dtrace probe with error reporting.
I hope user will smart enough to unmount and run fsck, if will get errors on directory listing.
Comment Actions
Thanks!
The printf vs dtrace call choice is not easy but yes as long as the filesystem is still in usable condition I think we should prefer the dtrace call.
You may want to send a diff to @mckusick syncing the UFS change. FWIW, I think UFS should also embrace DTrace ;).
Comment Actions
I thought about this patch little bit more and found, that it is not so simple to not panic in all places.
First place:
/*
* Check that directory length properly reflects presence
* of this entry.
*/
if (entryoffsetinblock + EXT2_DIR_REC_LEN(ep->e2d_namlen) >
dp->i_size) {
ext2_dirbad(dp, i_offset, "i_size too small");
brelse(bp);
return (EIO);
}
brelse(bp);It is possible do not panic in this case, because it is just incorrect i_size and could be simple fixed by fsck.
Second place:
if (ext2_check_direntry(vdp, ep, offset)) {
int i;
ext2_dirbad(ip, *offp, "mangled entry");
i = bsize - (offset & (bsize - 1));
*offp += i;
offset += i;
ep = (struct ext2fs_direct_2 *)((char *)data + offset);
continue;
}Ok, the corrupted entry was detected, just skip it and proceed lookup, panic not needed.
Third place:
if (namlen != 2 ||
dirbuf->dotdot_name[0] != '.' ||
dirbuf->dotdot_name[1] != '.') {
ext2_dirbad(xp, (doff_t)12,
"rename: mangled dir");Here things become more complex.
It is possible to return error here, but the renamed directory entry is already inserted to
new parent directory.
Here will be next picture:
root@fb:/mnt # mkdir -p ./DIR0/dirbad2 ./DIR1 # dirbad2 name will cause corrupted entry injection
root@fb:/mnt # tree
.
├── DIR0
│ └── dirbad2
├── DIR1
└── lost+found
5 directories, 0 files
root@fb:/mnt # mv ./DIR0/dirbad2 ./DIR1
mv: rename ./DIR0/dirbad2 to ./DIR1/dirbad2: Input/output error
root@fb:/mnt # tree
.
├── DIR0
│ └── dirbad2
├── DIR1
│ └── dirbad2
└── lost+found
6 directories, 0 files
So, the renamed directory entry will present both in old and new places. It is not so simple to revert newly inserted
directory entry from DIR1 in case of this error, at least, it is needed to implement function, which will iterate thru whole directory and
invalidate the inserted entry. The another way is to modify rename to validate this entry more earler and it is not so simple too.
In case of running fsck directory entry will be removed from DIR1 and will be fixed,
but, this situation with two same directories can confuse user more then panic call.
My proposition for now is to leave panic in third place, at least the propability of panics will be decreased.
Patch with corrupted entry injection code:
diff --git a/sys/fs/ext2fs/ext2_lookup.c b/sys/fs/ext2fs/ext2_lookup.c index 560a7c41cc38..a68cebf973b1 100644 --- a/sys/fs/ext2fs/ext2_lookup.c +++ b/sys/fs/ext2fs/ext2_lookup.c @@ -803,6 +803,8 @@ ext2_dirbad(struct inode *ip, doff_t offset, char *how) mp = ITOV(ip)->v_mount; + printf("ext2_dirbad():%s\n", how); + SDT_PROBE4(ext2fs, , trace, ext2_dirbad_error, mp->mnt_stat.f_mntonname, ip->i_number, offset, how); } diff --git a/sys/fs/ext2fs/ext2_vnops.c b/sys/fs/ext2fs/ext2_vnops.c index 67137c45b275..60b2a2f57db7 100644 --- a/sys/fs/ext2fs/ext2_vnops.c +++ b/sys/fs/ext2fs/ext2_vnops.c @@ -1399,6 +1399,19 @@ ext2_mkdir(struct vop_mkdir_args *ap) sizeof(struct ext2fs_direct_tail)); ext2_init_dirent_tail(EXT2_DIRENT_TAIL(buf, DIRBLKSIZ)); } + + /// -> dirbad1 set e2d_reclen not % 4 + if (memcmp(cnp->cn_nameptr, "dirbad1", cnp->cn_namelen) == 0) { + printf("==== dirbad1 injected: ino=%lu\n", ip->i_number); + dirtemplate.dotdot_reclen -= 1; + } + + /// -> dirbad3 set incorrect type for '..' + if (memcmp(cnp->cn_nameptr, "dirbad2", cnp->cn_namelen) == 0) { + printf("==== dirbad2 injected: ino=%lu\n", ip->i_number); + dirtemplate.dotdot_type = 15; + } + memcpy(buf, &dirtemplate, sizeof(dirtemplate)); ext2_dirent_csum_set(ip, (struct ext2fs_direct_2 *)buf); error = vn_rdwr(UIO_WRITE, tvp, (caddr_t)buf, @@ -1418,6 +1431,12 @@ ext2_mkdir(struct vop_mkdir_args *ap) ip->i_flag |= IN_CHANGE; } + /// dirbad0 -> set i_size less than first dirblock + if (memcmp(cnp->cn_nameptr, "dirbad0", cnp->cn_namelen) == 0) { + printf("==== dirbad0 injected: ino=%lu\n", ip->i_number); + ip->i_size = 3; + } + #ifdef UFS_ACL if (dvp->v_mount->mnt_flag & MNT_ACLS) { error = ext2_do_posix1e_acl_inheritance_dir(dvp, tvp, dmode,
Comment Actions
In UFS/FFS we handle the mangled '..' entry by silently ignoring the error and proceeding with the removal of the old directory entry. That leaves the only cleanup for fsck is to fix the broken '..' entry. I think that is what you should do too.
Now that dirbad no longer panics, I'll make the change to call it when the ".." update fails when I make these corresponding changes in UFS/FFS.
Comment Actions
Ok, let's ignore mangled dir entry on rename.
The situation will looks like:
root@fb:/mnt # mkdir -p DIR0/dirbad2 DIR1 root@fb:/mnt # mv ./DIR0/dirbad2 ./DIR1 root@fb:/mnt # tree . ├── DIR0 ├── DIR1 │ └── dirbad2 └── lost+found 5 directories, 0 files root@fb:/mnt # cd ~ root@fb:~ # umount /mnt root@fb:~ # e2fsck -f -y /dev/md0 e2fsck 1.47.0 (5-Feb-2023) Pass 1: Checking inodes, blocks, and sizes Pass 2: Checking directory structure Second entry '..^@^@^@^@^@^@^@^@^@^@^@^@^@' (inode=40961) in directory inode 40962 should be '..' Fix? yes Pass 3: Checking directory connectivity '..' in /DIR1/dirbad2 (40962) is <The NULL inode> (0), should be /DIR1 (8193). Fix? yes Pass 4: Checking reference counts Inode 2 ref count is 4, should be 5. Fix? yes Inode 8193 ref count is 4, should be 3. Fix? yes Pass 5: Checking group summary information /dev/md0: ***** FILE SYSTEM WAS MODIFIED ***** /dev/md0: 15/65536 files (0.0% non-contiguous), 13022/262144 blocks root@fb:~ # e2fsck -f -n /dev/md0 e2fsck 1.47.0 (5-Feb-2023) Pass 1: Checking inodes, blocks, and sizes Pass 2: Checking directory structure Pass 3: Checking directory connectivity Pass 4: Checking reference counts Pass 5: Checking group summary information /dev/md0: 15/65536 files (0.0% non-contiguous), 13022/262144 blocks root@fb:~ # mount -t ext2fs /dev/md0 /mnt root@fb:~ # cd /mnt root@fb:/mnt # tree . ├── DIR0 ├── DIR1 │ └── dirbad2 └── lost+found 5 directories, 0 files
So, the issue could be correctly fixed by fsck.
Comment Actions
Looks ready to go. Happy the change ended up so simple. I have the equivalent changes ready for UFS/FFS:
diff --git a/sys/ufs/ufs/ufs_lookup.c b/sys/ufs/ufs/ufs_lookup.c index 4c390f4c42ef..cabd04a50bfd 100644 --- a/sys/ufs/ufs/ufs_lookup.c +++ b/sys/ufs/ufs/ufs_lookup.c @@ -548,9 +548,8 @@ ufs_lookup_ino(struct vnode *vdp, struct vnode **vpp, struct componentname *cnp, */ if (i_offset + DIRSIZ(OFSFMT(vdp), ep) > dp->i_size) { ufs_dirbad(dp, i_offset, "i_size too small"); - dp->i_size = i_offset + DIRSIZ(OFSFMT(vdp), ep); - DIP_SET(dp, i_size, dp->i_size); - UFS_INODE_SET_FLAG(dp, IN_SIZEMOD | IN_CHANGE | IN_UPDATE); + brelse(bp); + return (EIO); } brelse(bp); @@ -758,17 +757,10 @@ ufs_lookup_ino(struct vnode *vdp, struct vnode **vpp, struct componentname *cnp, void ufs_dirbad(struct inode *ip, doff_t offset, char *how) { - struct mount *mp; - mp = ITOV(ip)->v_mount; - if ((mp->mnt_flag & MNT_RDONLY) == 0) - panic("ufs_dirbad: %s: bad dir ino %ju at offset %ld: %s", - mp->mnt_stat.f_mntonname, (uintmax_t)ip->i_number, - (long)offset, how); - else - (void)printf("%s: bad dir ino %ju at offset %ld: %s\n", - mp->mnt_stat.f_mntonname, (uintmax_t)ip->i_number, - (long)offset, how); + (void)printf("%s: bad dir ino %ju at offset %ld: %s\n", + ITOV(ip)->v_mount->mnt_stat.f_mntonname, (uintmax_t)ip->i_number, + (long)offset, how); } /* diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c index ae6d963920f3..54046c285fd7 100644 --- a/sys/ufs/ufs/ufs_vnops.c +++ b/sys/ufs/ufs/ufs_vnops.c @@ -1730,7 +1730,9 @@ ufs_rename( /* Journal must account for each new link. */ softdep_setup_dotdot_link(tdp, fip); SET_I_OFFSET(fip, mastertemplate.dot_reclen); - ufs_dirrewrite(fip, fdp, newparent, DT_DIR, 0); + if (ufs_dirrewrite(fip, fdp, newparent, DT_DIR, 0) != 0) + ufs_dirbad(fip, mastertemplate.dot_reclen, + "rename: missing ".." entry"); cache_purge(fdvp); } error = ufs_dirremove(fdvp, fip, fcnp->cn_flags, 0);