In D26243#587174, @shivank wrote:

In D26243#587132, @mjg wrote:

Audit support for regular lookup starts with AUDIT_ARG_UPATH1_VP/AUDIT_ARG_UPATH2_VP without any vnodes locked. Later on visited vnodes get added with AUDIT_ARG_VNODE1/AUDIT_ARG_VNODE2 which only performs VOP_GETATTR (i.e. does *NOT* resolve any paths). Your code should follow the same scheme.

As you can see path resolving routines can take vnode locks on their own (modulo the smr case). This means they can't be called with locked vnodes to begin with, as otherwise you risk violating global lock ordering and consequently deadlocking the kernel.

The VOP_ISLOCKED routine is not entirely legal to call if you don't hold the lock. The name is perhaps misleading, but it can only reliably tell you that you have an exclusive lock or that *SOMEONE* has a shared lock (and it may be you). Or to put it differently, if you don't have the vnode locked but someone else has it shared locked, you will get non-0 and that's how you get the panic. Regardless of this problem, adding the call reduces performance and most notably suggests a bug on its own.

So the question is why are you calling here with any vnodes locked.

I wish to audit the canonical path of the file requested by the NFS clients. The requested path from the client is extracted in the NFS server using nfsrv_parsename, but the vnode is locked in some NFS services. I thought of unlocking/relocking of vnode for path audit but Rick advised not to. That's why I had to call this locked vnode.

Thanks for your question which made me rethink the problem from scratch and I got a new idea for auditing path.

Hi @rmacklem and @asomers, if I use nfsvno_namei to get the canonical path for the client, I will not the need the AUDIT_ARG_UPATH1_VP.which will save me from all the trouble of passing locked vnode to vn_fullpath_global. Please provide your opinion on the same.

ping @pi @trasz . Are either of you able to review this change?

Yeah, doing copies 2MB at a time shouldn't be too slow. However, it does cause a different problem, I think. When I used this to copy a 16 GB sparse file (512B used) on UFS, the output file had 32MB used. I'm guessing that's due to UFS indirect blocks, and that increasing the copy_file_range bufsize would reduce the number of indirect blocks created. Alternatively (and probably better) would be for cp to use SEEK_HOLE/SEEK_DATA to completely skip the holes. But this version is still a lot better than the previous one.

The new code looks better. But grrr, there are two big problems:

1. It doesn't compile due to some recent changes on head. I suggest the following:
   • Remove the <rpc/rpc.h>, <sys/mount.h>, and <fs/nfs/*> includes from audit.h. In addition to fixing the compile failure, it's generally not recommended to include headers from other headers. Sometimes it's necessary, but it also causes header pollution, and slow build times. Instead of including those files, just forward declare struct nfsrv_descript; and struct kaudit_record;.
   • Add `<netinet/in.h>, <rpc/rpc.h>, <fs/nfs/nfsdport.h>, <fs/nfs/nfsproto.h>, and <fs/nfs/nfs.h> to audit_bsm_db.c
   • Add <rpc/rpc.h>, <fs/nfs/nfsport.h>, <fs/nfs/nfsproto.h>, and <fs/nfs/nfs.h> to audit.c

In D26184#581938, @se wrote:

I have added the configuration option "negative-confidence-threshold" to nscd in r238094 (in 2012), but since the default was kept at "1", this change has not become visible in practice.

It should work (did at that time in my tests), but I have received neither negative nor positive reports of its fitness for the intended purpose.

A value of 2 for this parameter ought to prevent the issue of entries being probed, not found, added, and then still not found due to the negative cache entry from the first probe.

This adds the cost of one additional negative lookup for each non-existent entry, which might be or might be not preferable to first looking up the file.

But it should help if new entries are added to other database types than local files (e.g., to NIS), a case not covered by the suggested change to the lookup order. (And having "cache" behind "nis" would negate its usefulness.)

I'm not opposed to changing the order from "cache files" to "files cache" (assuming that the file being read is on a local file system, not on e.g. NFS in case of a disk-less client), but I'd want to suggest that the "negative-confidence-threshold" was tested (in different settings) and proposed as a possible solution for this use case, too.

If the RFC does not allow unconfirmed clients to have delegations, then I think this change is correct. And I can't find any other problems with it.

If it makes a difference, one of the crashed server's clients _did_ recently do a change of address (though I'm unsure of the exact timing, and of course I can't guarantee that the client isn't buggy). But none of the clients rebooted within 4 days of the crash.

This seems wrong for two reasons:

• nfsrv_docallback has other callers, too
• How can we be sure that the previous client isn't still using its delegation?

In D25869#583083, @shivank wrote:

It looks like your most recent change rebased the base revision. That makes it very hard to see which changes are from you and which aren't. Could you please either un-rebase it or, if that's not possible, open a new review?

Ohh, Sorry! I didn't thought pulling HEAD changes will create this side-effect in revision
I think I would open a new review as going back will have conflicting changes again. Should I abandon this while creating a new one??

Using two completely separate functions reduces the scope of error. Also prevent any mutation to the current code path for not locked vnodes, while allowing it to work for locked vnodes.

Fix build on powerpc and fix one style bug

In D26034#580088, @mmacy wrote:

LGTM

ping. Any comments here? I'd like to commit before 12.2-RELEASE if possible.

ping. Any further comments on this PR?

This is a much better locking strategy. However, there's a lot of duplicated code. Could you maybe combine the _locked with the original functions, so there wouldn't be so much duplication?

Fix crypto_cursor_advance when amount > 4096

• Update man pages

In D25671#577993, @jhb wrote:

Somehow my earlier comment was stuck. There are still some style nits about leading spaces before block comments (and before the new "skip" macro in criov.c). I think the general change looks good. I think it would be good to commit the new buffer type separately from the geli changes (so two commits). I will probably test this for KTLS in the near future.

Don't duplicate USE=kmod's IGNORE warning

In D26033#577541, @freqlabs wrote:

Shouldn't the USES=kmod bit handle this? Mk/Uses/kmod.mk

ping. Is there anything else I need to do here?

Ok. As long as you've tested it with GCC6, then it's fine by me.

r329984 was done for a reason. With your change, can you still build world with GCC6?

Restore gordon and knu

Thanks for catching those. My process is only partially automated.

Use CRYPTO_BUF_VMPAGE instead of CRYPTO_BUF_UNMAPPED, and name static functions cvm_page_XXX

Minimize conditional compilation, and style fix to "char*"

In D25671#570483, @markj wrote:

• Avoid conditionally defining struct members and code when possible, especially in headers. This removes the need to import machine/param.h in crypto headers.

Respond to kib's comments, mostly style. Also, rebase.

Conditionally compile most CRYPTO_BUF_VMPAGE code

Allow PMAP_HAS_DMAP to change at runtime

In D25671#569908, @kib wrote:

Now I am confused. It seems that the new patch works on the page at the time, unless I missed a detail. Then, if we return to the design where you used sf bufs, you actually do not need more then one sf_buf at the time. I am not stating that it is possible to changing the patch back to sf_bufs, since it might be impossible to use sleepable allocation, in which case you would need to retract to transient mapping. But if this is not issue, then 'single sf buf at a time' might be an option.

Use a vm_page_t array instead of sf_buf, only on DMAP architectures.

Fix deadlock during out-of-sf_buf condition

Add sf_buf intelligence to crypto(9) and use it for geli reads

In D25671#568830, @kib wrote:

In D25671#568758, @asomers wrote:

Ok, you two successfully guilt tripped me. I went ahead and did it the way you suggested, adding sf_buf intelligence to crypto(9) and using it for reads (but still using uiomove for writes). It does improve read performance with larger block sizes. But curiously it only works when the aesni module is loaded. Without that module, using pure software crypto, I get miscompares on every read. I'm betting that one of you two already knows the answer why. Could you fill me in?

I cannot debug the code by description of it.