rpc: Improve socket locking in svc_vc_accept()
ClosedPublic
Actions

Authored by markj on Tue, Feb 10, 11:29 PM.

Details

Reviewers

glebius
rmacklem

Commits

rG7c1c0e94363d: rpc: Improve socket locking in svc_vc_accept()

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 70619
Build 67502: arc lint + arc unit

Event Timeline

markj created this revision.Tue, Feb 10, 11:29 PM

Herald added a subscriber: imp. · View Herald TranscriptTue, Feb 10, 11:29 PM

markj requested review of this revision.Tue, Feb 10, 11:29 PM

Harbormaster completed remote builds in B70615: Diff 171716.Tue, Feb 10, 11:29 PM

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)

This revision is now accepted and ready to land.Wed, Feb 11, 12:55 AM

markj added inline comments.Wed, Feb 11, 2:22 PM

sys/rpc/svc_vc.c
392–397	This line also doesn't make sense. It always sets `so_state = 0`.

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

This revision now requires review to proceed.Wed, Feb 11, 2:23 PM

Harbormaster completed remote builds in B70619: Diff 171760.Wed, Feb 11, 2:23 PM

glebius added inline comments.Thu, Feb 12, 12:31 AM

sys/rpc/svc_vc.c
395	Isn't this change going to clear SS_NBIO in the case it was originally set?

Sorry, missed the if (nbio == 0)

glebius accepted this revision.Thu, Feb 12, 12:33 AM

This revision is now accepted and ready to land.Thu, Feb 12, 12:33 AM

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!

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.

Closed by commit rG7c1c0e94363d: rpc: Improve socket locking in svc_vc_accept() (authored by markj). · Explain WhyThu, Feb 12, 2:08 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rG7c1c0e94363d: rpc: Improve socket locking in svc_vc_accept().