rpc: Improve socket locking in svc_vc_accept()

[markj]

Summary

so_state modifications must be synchronized by the socket lock. For the
listening socket this probably doesn't matter but for the child socket I
think it's possible that this unlocked update clobbers a state
transition if the nascent connection is being disconnected for some
reason.

Untested, I just spotted this while reading code, staring at PR 292884.

[rmacklem]

This looks fine to me.
I will note that the NFS server never uses svc_vc_accept().
(The accept() is done in userspace.)

It might get used by the NLM. (rpc.lockd)

[markj]

sys/rpc/svc_vc.c

392–397

This line also doesn't make sense. It always sets so_state = 0.

[markj]

• Set SS_NBIO on the child in solisten_dequeue()
• Fix clearing SS_NBIO in the listening socket

[glebius]

sys/rpc/svc_vc.c

395

Isn't this change going to clear SS_NBIO in the case it was originally set?

[glebius]

Sorry, missed the if (nbio == 0)

[rmacklem]

I probably should resign, since I don't understand
when/if ACCEPT4_INHERIT might be specified
as an argument for solisten_dequeue()?

I put a printf() in svc_vc_accept() and I have not yet
seen it, including when I did a NFSv3 mount with rpc.lockd
running, but I can only do localhost mounts, because I
am not at home (and won't be for weeks).
So, I don't know when the function might ever be called?
(I know it isn't called for NFS, but am not sure for rpc.lockd.)

sys/rpc/svc_vc.c

392–397

Yea, I suspect the original line was meant to
be head->so_state &= (nbio | ~SS_NBIO);.
(I recall "&" when it should be "|" in other places
in the code.)

Good catch!

[markj]

In D55247#1263650, @rmacklem wrote:

I probably should resign, since I don't understand
when/if ACCEPT4_INHERIT might be specified
as an argument for solisten_dequeue()?

ACCEPT4_INHERIT is only used to implement the accept4(2) system call, apparently. solisten_dequeue() is a rather weird interface, it should be probably define its own flags rather than mixing SOCK_NONBLOCK, ACCEPT4_INHERIT, etc..

I put a printf() in svc_vc_accept() and I have not yet
seen it, including when I did a NFSv3 mount with rpc.lockd
running, but I can only do localhost mounts, because I
am not at home (and won't be for weeks).
So, I don't know when the function might ever be called?
(I know it isn't called for NFS, but am not sure for rpc.lockd.)

I don't see any calls on my NFS server (running rpc.lockd) when mounting from a different host. So maybe this code really is unused. I'm not sure how to prove that though.