unionfs: fix NULL deref on closing an fd passed through SCM_RIGHTS

[jah]

Summary

If the last reference to an open file is contained in an SCM_RIGHTS
message in a UNIX domain socket, and that message is discarded without
being read out by the receiver, VOP_CLOSE will ultimately be called
with ap->a_td == NULL.

Change unionfs_close() to check for this condition instead of blindly
passing the thread to unionfs_find_node_status() which will try to
dereference it. Also add relevant asserts on the node status lookup
paths.

PR: 289700
Reported by: asomers
MFC after: 3 days

[jah]

I did post some related questions to https://reviews.freebsd.org/D52625 about both the policy of passing NULL for the thread to VOP_CLOSE and the possibility of trying to make the passing of file objects to vnode ops more consistent.
But assuming everyone's ok with still passing NULL there, I'll also update VOP_OPENCLOSE.9 to note that possibility.

[glebius]

I trust Alan's expertise here. IMHO, given that this patch is analog to fusefs one, should be fine. But not expert in the area.

[asomers]

In D53079#1212648, @glebius wrote:

I trust Alan's expertise here. IMHO, given that this patch is analog to fusefs one, should be fine. But not expert in the area.

It's a stretch to say that I have expertise here. I've never made any changes to unionfs before.

[jah]

In D53079#1212688, @asomers wrote:

In D53079#1212648, @glebius wrote:

I trust Alan's expertise here. IMHO, given that this patch is analog to fusefs one, should be fine. But not expert in the area.

It's a stretch to say that I have expertise here. I've never made any changes to unionfs before.

FWIW, before I posted this review I did create a simple test program using the scm_rights test Alan added in https://reviews.freebsd.org/D52625 as a template. I verified that it panicked as expected without the patch and worked correctly with the patch.

[olce]

This change is an improvement without drawbacks, so should be committed.

See also my comment in D53079.

For the time being, td passed to VOP_CLOSE() can be NULL, and indeed it would be great to document it. I don't think that's the case for VOP_OPEN(), but haven't checked thoroughly.

[olce]

sys/fs/unionfs/union_vnops.c

817–818

Matter of taste, but for a single assignment in if branches, I would rather use the ternary operator. Else, I would prefer that the superfluous braces are removed.