Hello FreeBSD maintainers,

I've tried to justify the current implementation as best I can to see if it can get past the finish line instead of going for a credfd solution (which I don't think I'll have the will to design and implement).
For the credfd solution, I agree with the wins, although I think it would be expensive to prepare everything upfront, also that design doesn't solve the copy-on-write problem you mentioned.

In D18930#406796, @mjg wrote:

I don't have an opinion about the userspace API. It does seem a little bit fishy that there is no tight control from that end. I would expect a fully-privileged daemon to get create a credfd an allow certain uids/gids to be switched to. Then it can drop privs. But again, I did not think this through.

Some updates:

@mjg: reverted back to one reference per object. You're right, better safe than sorry.

The reference hold/free logic was wrong. now in the case where td_ucred==td_pucred only one reference is held.

@mjg: good call, this revision is likely the best regarding performance, and code complexity. As for future-proofing, it's obvious that process credentials will always be necessary for backwards compatibility so caching them entirely in each thread is a good idea.

@mjg: moved copyout outside of PROC_LOCK for getgroups, getresuid and getresgid.

Moved the process to thread credential syncronization logic into thread_cow_update instead of everywhere scattered around the codebase.

@rwatson: The patch has been updated to take into account the other parts of credentials: jail, MAC, audit and flags are written to thread credential in order for them to always be in sync with the process credentials.

Is this change safe without holding the process lock? Historically, p_ucred hasn't been a stable pointer without it.

The dot special behaviour was because our application is currently using getfhat with a NULL path. We'll fix this in the application rather than in kernel code. I've rolled back to a behaviour similar to openat, also amended the manpage.

No more locks, no more leaks as far as I can tell

In D18359#391303, @kib wrote:

In D18359#391289, @jack_gandi.net wrote:

I was able to reproduce the lock recursion in the case where the source fhp points to the same object as the destination link. It turns ou the link(2) doesn't lock the source node which seems acceptable. So I changed fhlinkat_kern to only LK_SHARED on the source node, therefore the kern_linkat_vp doesn't need the bool locked anymore because all the callers have the same input locking state.

LK_SHARED is still locking the vnode, although in the shared mode. So from the brief look at the updated patch, you are leaking the vnode locks.

Do you run the tests with WITNESS + DEBUG_VFS_LOCKS kernel options ?

I was able to reproduce the lock recursion in the case where the source fhp points to the same object as the destination link. It turns ou the link(2) doesn't lock the source node which seems acceptable. So I changed fhlinkat_kern to only LK_SHARED on the source node, therefore the kern_linkat_vp doesn't need the bool locked anymore because all the callers have the same input locking state.

temporary pathc, I'm still looking into the lock recursion that @kib has mentionned.

In this diff there is no more code duplication. also I forgot to include the two new manpages in the previous versions (fhlink.2 and fhreadlink.2).

Here's some more context to the diff. I wasn't aware of style(9), will check that before submitting in the future.

In D17917#384287, @kib wrote:

Note the request for attributes/metadata for the change. I will commit after you provide the info.

In D17917#383965, @kib wrote:

In D17917#383881, @jack_gandi.net wrote:

In D17917#383719, @kib wrote:

Also please add an implementation note to mention readdir plus NFS RPC response, as suggested by Rick.

I'm not sure what you mean by this, should I add a new section to the manpage?

Yes, this is most suitable for 'IMPLEMENTATION NOTES section.

In D17917#383719, @kib wrote:

Also please add an implementation note to mention readdir plus NFS RPC response, as suggested by Rick.

Added to the getdirentries.2 man page. Also make udf_readdir more consistent as requested. Apparently udf is used for DVDs and optical disks so it shouldn't matter anyway.

In D17917#382778, @pfg wrote:

Maybe a sweep with opengrok is a good idea to see where we should be using this.

overlooked a zfsctl_snapdir_readdir vfs call used for zfs snapshots.

this is a rough check I used for testing. Basically we want the d_off value to be the lseek value *after* reading a given entry. To check for lseek we must only iterate over getdirentries one entry at a time, otherwise the lseek offset will be too far off.