This code will only audit one of the two nd_nam fields. But both fields can be used at the same time. Can you audit both instead?

As below, I suggest moving this line into audit_record_ctor

Instead of allocating an audit record for every RPC, could you instead store an audit record for every nfsd thread, and reuse the storage between RPCs? That would be more efficient.

I can't understand what you mean here. In any case, this must be cleaned up before merging.

Can you move this line to audit_nfsrecord_ctor? That would eliminate the possibility of creating a record and freeing it while the kaudit_record_type field is still invalid.

Is this lookup table valid for NFS protocols 2, 3, and 4?

I recommend a KASSERT in the default position.

Users will want to see the file name. Can you log that, or at least log enough information that the user can reconstruct it?

nd.nd_nam = svc_getrpccaller(rqst);
nd.nd_nam2 = rqst->rq_addr;

and,

#define svc_getrpccaller(rq)					\
	((rq)->rq_addr ? (rq)->rq_addr :			\
	    (struct sockaddr *) &(rq)->rq_xprt->xp_rtaddr)

So, when(TCP) nd_nam2 == NULL, nd_nam is client sockaddr. And, when(UDP) nd_nam2 != NULL, nd_nam and nd_nam2 are same. Therefore, Only one need to be audited.

Can you please give some hints on how it can be done?

No, It's valid for v2 and v3 only. v4 is a little different in terms of nd_procnum, Hence, the table is not valid for NFSv4. Also, NFSv4 audit support is yet to be implemented.

I use panic(9) since no expression to evaluate?

Not all RPCs have a filename in their arguments. How can I extract the filename rpc request in NFS server-side? for instance, some nfsrvd_read/write have filehandle as an argument for referencing the file.

Hm, it looks like nd_nam2 is really just a glorified boolean: it's either null or not null. So we could audit it that way. The comment says that it indicates whether the mount uses TCP. But from a security auditing perspective, who cares? I think you should just ignore it.

I was hoping that there would already be some kind of struct describing each nfsd worker thread. But I can't find one. Do you know of any?

Good. Now we'll get to find out if somebody has figured out how to mount NFS over bluetooth ;)

It's hard and not always possible. And we want to avoid doing too much work while auditing. That's why it would be acceptable merely to log enough information that a user can reconstruct the file name later. For example, if LOOKUP supplies a filename and returns some kind of opaque file handle, then READ supplies that same file handle, the user can deduce what was read as long as the audit trail contains the filename for LOOKUP and the file handle for both LOOKUP and READ.

Yep. If you really want the history behind this, it goes something like this...
A few years ago when this was written (about 1987), UDP passed the client
address up through soreceive() as an mbuf of type MT_SONAME, so nd_nam2
was an mbuf ptr.

For TCP (it was the first NFS over TCP implementation ever done as far as I know),
nd_nam was filled in by the kernel variant of getpeername() and was a struct sockaddr *.

Since the code used the client address for exports checking, the one in the mbuf
(nd_nam2) was copied to nd_nam to simplify this checking.

This was all long before the kernel RPC came along and nd_nam2 has just lived on.
(It is filled in from a slightly different place in the krpc code, but I don't believe it
can ever be different than nd_nam for UDP and is just NULL for TCP.)

Yes. All of these functions check for the ESTALE error case at
the beginning of them (line#247 below for example).

It might be better to put this call after the error check.
(after line#248 below, for example)

As noted in a separate discussion, unlocking and relocking of a directory
vnode needs to be very carefully evaluated.

For example, at this time, the path has already been put through a "lookup"
and the file object does not exist here.

However, unlocking/relocking allows another thread to create an object of
the same name (not necessarily the same object or type of object).

In general, I am opposed to unlocking/relocking vnodes in the code,
except at the very beginning of an RPC/operation.

Same as above w.r.t. unlocking/relocking a directory vnode.

Same as above...

bz@ likes anything based on AF_INET or AF_INET6 to be #ifdef INET
or #ifdef INET6 even if it builds without that.
(It documents where the code fragments are and he dreams of getting
rid of INET someday, although I doubt that will be in my lifetime.)

I'll admit I do not understand what this is trying to do.

I do not see any reason to vref()/vrele() the vnode, since
there must be a v_usecount held by the function where this
is called and that will not go away.

Also, VOP_ISLOCKED() returns the type of lock held.
The code needs to look something like:

locktype = VOP_ISLOCKED(vp);
/* As well as returning 0 for unlocked, it can return LK_EXCLOTHER for

another thread holding a lock on it. */

if (locktype != LK_EXCLUSIVE && locktype != LK_SHARED)

• lock it

then if you locked it

VOP_UNLOCK(vp);

Then, if you do the vn_lock() without LK_NOWAIT as you have done now,
you must be careful to never call this when another vnode lock is held,
since ordering of vnode locking if you are to lock multiple vnodes must
be carefully designed to avoid deadlocks. (There are very few people in
the world, and I am not one of them, who actually understand how to safely
to lookup and rename of paths the intersect, as an example.)

The safe way to this, which I would be comfortable with, would be a

vn_lock(vp, LK_SHARE | LK_NOWAIT);
then if it fails you can't get all the audit information.

--> I'm guessing you would still want to generate an audit record, but it

won't be able to hold vnode details.

There is a problem with calling AUDIT_NFSARG_UPATH1_VP at the beginning (as you suggested to do it in nfsd_fhtovp in the mail) of the RPC operation: I don't have the pathname and rootdir info at that point, since the RPC extracts those data in nfsrvd_*.
I think I should drop the unlock/relock idea and define two separate path for the locked and unlocked vnode case.
I earlier added if (VOP_ISLOCKED(vp)) checks in vn_fullpath_any in vfs_cache.c, but that also added complications and bugs in code. Splitting it into the separate would simplify it.

Can you please suggest something here, rmacklem@?

I did the #ifdef INET... #endif part. But something is wrong somewhere as it skips the block and jumps to the default case. (pointing the INET/INET6 is not defined)
Am I missing some headers file?

if I don't use vref/vrele the kernel crashes - panic: condition vp->v_holdcnt > 0 not met at /usr/home/shivank/freebsd/sys/kern/vfs_vnops.c:1700 (_vn_lock)

I did the suggested changes for locking.

Can vattr.va_fileid from the vnode attr token be used to reconstruct the file name later? In that case, filehandle info would not be needed.
I checked manually for some events and I think it should work. What do you think?

The naming convention for functions like this is to put the _locked at the end, like vn_fullpath_any_locked

What would you do differently if LK_EXCLOTHER were set?

There's a lot of duplicated code here. Could you combine it with vn_fullpath_dir into a single function that takes an optionally-locked vnode?

Is it really possible to reach this line? Shouldn't lktype always be non-zero in this function?

Since vn_locked_vptocnp is only used in a single location, you can make it a static function, and not declare it in the header.

I request Rick to please guide on this. In one of his other comment, he wrote "/* As well as returning 0 for unlocked, it can return LK_EXCLOTHER for another thread holding a lock on it. */ if (locktype != LK_EXCLUSIVE && locktype != LK_SHARED". I didn't completely understand it, So wrote a comment here.

I observed the value of the vnode vp pointer can be changed by the vn_vptocnp function call. That's why I check VOP_ISLOCKED whenever such an assignment can happen.
Please correct me if I have any misunderstanding of this concept.

What's different about NFSv4 here?

NFSv4 audit support is in separate feature branch here. Due to NFSv4's slightly different nature ( 1 compound RPC and numbers of suboperations in it ), I audit each of its sub-operation in nfsrvd_compound() by adding AUDIT_NFSRPC_ENTER/EXIT like this.

I get it. NFSv4 goes through this code path, but it's pointless to audit arguments for the compound RPC. You'll audit the operations instead.