Implement fdescfs for Linuxulator

[kib]

The main question is perhaps asked in the UCB copyright comment.

sys/compat/linfdescfs/linfdesc.h

6 ↗

(On Diff #30327)

Was it ? Also te UCB copyright assignment looks doubtful.

So globally this looks like a copy/paste of fdescfs(5). Why it is needed ? Please explain where the behavior of the new fs differs from native fdescfs. Then I will ask another question.

37 ↗

(On Diff #30327)

Why this protection is needed ?

Also, there is no double-include protection.

sys/compat/linfdescfs/linfdesc_vfsops.c

72 ↗

(On Diff #30327)

Blank line after { is needed.

return (kernel_mount());

101 ↗

(On Diff #30327)

Why this cast is needed ?

125 ↗

(On Diff #30327)

Remove init from declaration, collapse two int vars declarations into single.

151 ↗

(On Diff #30327)

What is the purpose of taking the lock there ?

239 ↗

(On Diff #30327)

Why this dependency is needed ?

sys/compat/linfdescfs/linfdesc_vnops.c

374 ↗

(On Diff #30327)

Can you explain this ?

[dchagin]

Fixed most of the comments.

Reply to the main question. Yes, this code copied from FreeBSD fdescfs code.
Unlike FreeBSD, the Linux fdesc fs is a directory containing a symbolic links to the actual files, which the process has open.
A readlink(2) call on this file returns a full path in case of regular file or a string in a special format (type:[inode], anon_inode:<file-type>, etc..)
As well as in a FreeBSD, opening the file in the Linux fdescfs directory is equivalent to duplicating the corresponding file descriptor.

Here we have mutually exclusive requirement:
in case of readlink(2) Linux fdescfsw lookup() method should return VLNK vnode otherwise our kern_readlink() fail with EINVAL error.
In other calls Linux fdescfsw lookup() method should return non VLNK vnode.

For that I added new Linux thread flag LINUX_TD_READLINK, set it before calling kern_readlinkat() and in lookup() mthod return VLNK vnode if this flag is set.

[dchagin]

sys/compat/linfdescfs/linfdesc.h

6 ↗

(On Diff #30327)

Replaced by my copyright, is this good?

[kib]

In D11452#237523, @dchagin wrote:

Fixed most of the comments.

Reply to the main question. Yes, this code copied from FreeBSD fdescfs code.
Unlike FreeBSD, the Linux fdesc fs is a directory containing a symbolic links to the actual files, which the process has open.
A readlink(2) call on this file returns a full path in case of regular file or a string in a special format (type:[inode], anon_inode:<file-type>, etc..)
As well as in a FreeBSD, opening the file in the Linux fdescfs directory is equivalent to duplicating the corresponding file descriptor.

Here we have mutually exclusive requirement:
in case of readlink(2) Linux fdescfsw lookup() method should return VLNK vnode otherwise our kern_readlink() fail with EINVAL error.
In other calls Linux fdescfsw lookup() method should return non VLNK vnode.

For that I added new Linux thread flag LINUX_TD_READLINK, set it before calling kern_readlinkat() and in lookup() mthod return VLNK vnode if this flag is set.

Why don't you add a mount option to fdescfs to cause its VOP_READLINK() to always return what linux ABI expects ? Such fdescfs would be mounted under /compat/linux/dev/fd.

That means that LINUX_TD_READLINK is not needed at all.

sys/compat/linfdescfs/linfdesc.h

6 ↗

(On Diff #30327)

Probably no, you should added your copyright to the existing statement, but I think that this is moot.

[dchagin]

Modify native fdescfs to emulate Linux(4) ABI compatibility instead of writing separate Linux fdescfs

[dchagin]

Use v_vflag instead of v_vtype to store READLINK flag to avoid switch(v->v_vtype)

[kib]

sys/fs/fdescfs/fdesc_vnops.c

370

fdesc_allocvp() is much more suitable place for the flag assignment.

You need to filter out Froot fntype when setting the flag, otherwise panic() in readlink would be called.

430

Why don't you use vnode v_vflag there ? It would avoid two pointers indirections. I.e. I would write this fragment as

vap->va_type = (vp->v_vflag & VV_LINRDLNK) != 0 ? VLNK : VCHR;

542

Same there.

584

Do not put initialization into declaration block.

593

fdesc_readlink

597

At this moment, the vn vnode might be reclaimed after which vn->v_data points to freed memory.

619

Use strlen().

sys/sys/vnode.h

253

I suggest the VV_READLINK name.

[dchagin]

Fixed kib@ comments

[dchagin]

sys/fs/fdescfs/fdesc_vnops.c

542

VV_READLINK only for Fdesc node

[kib]

sys/fs/fdescfs/fdesc_vnops.c

542

I would prefer this assignment written with the ?:.

599

This cannot work as well. The process filedesc table is locked inside fget_cap(), but the thread already owns the vnode lock. So you get LOR there. Did you run this under WITNESS ?

Arguably, you need to read fd->fd_fd into a local variable and _then_ drop the vnode lock.

606

This vref() is useless, while fp is refed, vnode cannot be freed since fp also vrefed the vnode.

607

Again, vn_fullpath() cannot be called with a vnode locked, since it might need to lock some vnodes during the patch reconstruction, and this would break the vnode locks order.

[dchagin]

ugh, drop vnode lock before fget_cap() and vn_fullpath()

[kib]

sys/fs/fdescfs/fdesc_vnops.c

265

I still do not understand why don't you move this check and flag set to fdesc_allocvp().

[dchagin]

Move VV_READLINK flag set to the fdesc_allocvp()

[dchagin]

sys/fs/fdescfs/fdesc_vnops.c

265

Mostly due to two ways of success out from fdesc_allocvp(), one of them I do not understand how to test

[kib]

sys/fs/fdescfs/fdesc_vnops.c

247

I strongly prefer to have the vnode fully initialized before pushing it into the place where it can be found by other threads, even while it is locked.

There is clearly preferrable place where fs-private data for the vnode is constructed.

[dchagin]

Fix second return from fdesc_allovp()

[dchagin]

Ready

[kib]

sys/fs/fdescfs/fdesc_vnops.c

247

No, please re-read what I wrote.

Move the flag setting to line 200 (patched), where all other initialization of the fs-specific data is performed.

[dchagin]

ah, sorry, done. also fdesc_readdir() changed

[dchagin]

Committed at r321839