Page MenuHomeFreeBSD

acquire a reference count on the CLIENT structure for callbacks in the server krpc
ClosedPublic

Authored by rmacklem on May 6 2021, 10:09 PM.

Details

Summary

Michael Dexter <editor@callfortesting.org> reported
a crash in FreeNAS, where the first argument to
clnt_bck_svccall() was no longer valid.
This argument is a pointer to the callback CLIENT
structure, which is free'd when the associated
NFSv4 ClientID is free'd.

This appears to have occurred because a callback
reply was still in the socket receive queue.

This patch acquires a reference count on the CLIENT
that is not CLNT_RELEASE()'d until the socket structure
is destroyed. This should guarantee that the CLIENT
structure is still valid when clnt_bck_svccall() is called.
It also adds a check for closed or closing to
clnt_bck_svccall() so that it will not process the callback
RPC reply message after the ClientID is free'd.

Test Plan

Tested for both Linux and FreeBSD NFSv4 clients
with/without delegations enabled with a printf()
for the CLIENT reference count when svc_vc_destroy()
is called.
It worked as expected. No leak of M_RPC malloc()s
have been detected.

Michael Dexter <editor@callfortesting.org>
will hopefully be able to test this patch as well.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rmacklem created this revision.

Rick, do I understand right that after this there will be two different uses for xp_p2 (CLIENT * vs struct ct_data *) and supposedly those two never intersect? I don't remember much of this code, sorry.

Yes, the two ends of the RPC connection are separate. On the RPC client end
(which is the server end for callbacks, just to make it confusing),
xp_p2 still refers to the private data substructure under the CLIENT
structure and not the CLIENT itself.

When I first did the patch, I changed xp_p2 to refer to CLIENT instead
of the private data substructure on both ends, but took the RPC client end
changes out, because I thought it was unnecessary churn.

If you think changing both ends makes the code less confusing, I can
easily update the patch that way. It adds about 10lines of change,
but only changes the RPC client end semantics in that you use xp_p2->cl_private
instead of xp_p2 after it is changed.

Let me know if you think redefining xp_p2 for the RPC client end is preferred
and I'll update the patch.

Update comments to clarify what xp_p2 points to
on both the server (client end for callbacks) and
client (server end for callbacks).

No change other than comments.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 12 2021, 12:02 AM
This revision was automatically updated to reflect the committed changes.