Page MenuHomeFreeBSD

Fix Linux getttynam(3).
ClosedPublic

Authored by trasz on Jul 3 2020, 11:41 PM.

Details

Summary

Make linux stat(2) return the same st_dev for every devfs instance.
The reason for this is to work around an idiosyncrasy of glibc
getttynam() implementation: it checks whether st_dev returned for
fd 0 is the same as st_dev returned for the target of /proc/self/fd/0
symlink, and with linux chroots having their own devfs instance,
the check will fail if you chroot into it.

This is somewhat horrible, but I don't see a better way to fix it.

PR: kern/240767

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

trasz requested review of this revision.Jul 3 2020, 11:41 PM

I could understand what you are trying to do from the description, but then I completely confused by the patch. FreeBSD st_dev is mnt_stat.f_fsid, then linuxolator st_dev is lower 16bits of it. Now read the comment before vfs_getnewfsid().

In other words, I think it worked for you just because you only had less than 255 mounts total during the system life.

Hm, you're right. How about adding a reserved fsid (just pick a value and make vfs_getnewfsid() never use it), and using that for all S_ISCHR vnodes instead?

Hm, you're right. How about adding a reserved fsid (just pick a value and make vfs_getnewfsid() never use it), and using that for all S_ISCHR vnodes instead?

Better find somehow the canonical /dev mount and use its fsid (if found).

Use rootdevmp - set at boot, cleared at shutdown - instead of questionable
bit shuffling.

Relying on ISCHR/ISBLK is too broad. For instance, you can have character device on UFS of ext2fs or nfs. They are not interpreted as special vnode by FreeBSD kernel, but might have some meaning for linux userspace.

Instead of this, I suggest you to check the type of the backing filesystem in linux stat callback (you still have the vnode referenced there), and if it is from devfs mount, replace st_dev as you do.

Make the ajustments earlier, as suggested by kib@.

You are still translate based on the vnode type instead of type of the owning filesystem.

True. This also makes Linux stat(2) report different st_dev on devfs subdirectories and devfs device nodes.

What's the suggested way to determine if vnode belongs to devfs? Compare vp->v_mount->mnt_stat.f_fstypename with "devfs", and if matches cache vp->v_mount->mnt_stat.f_type to avoid string comparisons later on?

You can do something like vp->v_mount->mnt_vfc == rootdevmp->v_mount->mnt_vfc. In the case when you do not own the vnode lock, it is important to fetch vp->v_mount into a local variable and test it for NULL before dereferencing.

Check filesystem type instead of vnode type.

sys/compat/linux/linux_stats.c
139 ↗(On Diff #74164)

I think you should add __compiler_barrier() after this line, so that compiler is not allowed to reload v_mount later.

144 ↗(On Diff #74164)

(unrelated) This is very buggy. First, v_rdev is a member of union, it only means something for char vnodes. Second, it is unsafe, r_dev can be invalidated any time.

sys/compat/linux/linux_stats.c
144 ↗(On Diff #74164)

I wonder if I should just vn_lock() before doing all this?

Add __compiler_membar().

kib added inline comments.
sys/compat/linux/linux_stats.c
144 ↗(On Diff #74164)

vnode lock might be not enough to ensure stability of *v_rdev

Look at vn_isdisk() to how to access v_rdev safely.

This revision is now accepted and ready to land.Jul 10 2020, 9:51 AM