> On 11. Apr 2023, at 12:13, Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
>
> Last year hannken@ changed fstrans(9) to use a hash table mapping
> struct mount pointers to fstrans info, in order to fix a crash found
> by syzkaller arising from a race between fstrans_start(vp->v_mount)
> and vgone(vp):
>
> https://syzkaller.appspot.com/bug?extid=54dc9ac0804a97b59bc6
> https://mail-index.netbsd.org/source-changes/2022/07/08/msg139622.html
>
> However, while this may avoid a _crash_ from use-after-free of
> vp->v_mount->mnt_..., I'm not sure this is enough to prevent a _race_.
>
> Suppose we have the following sequence of events:
>
> lwp0 lwp1
> ---- ----
> mp = vp->v_mount;
> vgone(vp);
> -> vp->v_mount = deadfs
> unmount(mp)
> -> free(mp)
> mount a new file system
> -> malloc() returns the same mp
> fstrans_start(mp);
>
> Now lwp0 has started a transaction on some totally unrelated mount
> point -- and may take confused actions under the misapprehension that
> vp is a vnode that unrelated mount point.
This sequence is obviously racy, we (should) always use fstrans_start* like:
for (;;) {
mp = vp->v_mount;
fstrans_start(mp);
if (mp == vp->v_mount)
break;
fstrans_done(mp);
}
or
mp = vp->v_mount;
fstrans_start(mp);
if (mp != vp->v_mount) {
fstrans_done(mp);
return ENOENT;
}
thus avoiding this race and always holding fstrans on the right mount.
Some of the current uses need review though ...
--
J. Hannken-Illjes - hannken%mailbox.org@localhost
Attachment:
signature.asc
Description: Message signed with OpenPGP