* audit of the use of SVC_DROP in server reply path @ 2016-09-13 15:42 Chuck Lever 2016-10-07 21:28 ` Bruce Fields 0 siblings, 1 reply; 3+ messages in thread From: Chuck Lever @ 2016-09-13 15:42 UTC (permalink / raw) To: Bruce Fields; +Cc: Linux NFS Mailing List Hi Bruce- I think the two interesting cases are svc_set_client and svc_authorise. Who does a "goto dropit;" inside svc_process_common? • svc_set_client returns SVC_DROP • When svcauth_gss_set_client calls svc_unix_set_client, which can return SVC_DROP • When svc_unix_set_client calls cache_check, and it returns -EAGAIN • When svc_unix_set_client calls unix_gid_find, and it returns -EAGAIN • svcauth_gss_accept • when gc_proc == RPC_GSS_PROC_DATA or RPC_GSS_PROC_DESTROY, and gss_check_seq_num fails • when gc_proc == RPC_GSS_PROC_DESTROY and the result length is larger than a page • pc_func returns rpc_drop_reply - only used by NLM • vs_dispatch returns 0 • When nfsd_cache_lookup returns RC_DROPIT (the RPC is already in progress, or the client has retransmitted too soon): the server is going to reply anyway, safe to drop • pc_func returns nfserr_dropit (NFSv2's JUKEBOX) • RQ_DROPME is set (deferred requests?) • pc_encode is NULL - probably rare and inconsequential • svc_authorise returns non-zero • svcauth_gss_release returns a negative errno when integrity or privacy reply wrapping fails; i think this needs a connection reset • incoming RPC header is shorter than 24 bytes - connection reset would be better here anyway, IMO The question I have is what does SVC_CLOSE mean for a UDP transport? -- Chuck Lever ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: audit of the use of SVC_DROP in server reply path 2016-09-13 15:42 audit of the use of SVC_DROP in server reply path Chuck Lever @ 2016-10-07 21:28 ` Bruce Fields 2016-10-13 13:50 ` Chuck Lever 0 siblings, 1 reply; 3+ messages in thread From: Bruce Fields @ 2016-10-07 21:28 UTC (permalink / raw) To: Chuck Lever; +Cc: Linux NFS Mailing List Apologies for the delayed response: On Tue, Sep 13, 2016 at 11:42:37AM -0400, Chuck Lever wrote: > I think the two interesting cases are svc_set_client and svc_authorise. > > Who does a "goto dropit;" inside svc_process_common? > > • svc_set_client returns SVC_DROP > • When svcauth_gss_set_client calls svc_unix_set_client, which can return SVC_DROP > • When svc_unix_set_client calls cache_check, and it returns -EAGAIN > • When svc_unix_set_client calls unix_gid_find, and it returns -EAGAIN These should still result in a silent drop, since in these places we're still planning to reply--only after deferring and then revisiting the request later. (Though it might be worth verifying that there aren't any weird subcases where that isn't the case.) > • svcauth_gss_accept > • when gc_proc == RPC_GSS_PROC_DATA or RPC_GSS_PROC_DESTROY, and gss_check_seq_num fails This is the case that was causing us trouble. Should be a close. > • when gc_proc == RPC_GSS_PROC_DESTROY and the result length is larger than a page We're never going to reply, so close. > • pc_func returns rpc_drop_reply - only used by NLM Looks like this might happen in some cases on open? (Delegation conflict, or export lookup? I'm not sure.) In any case, this is NLM, which I'm guessing can deal with us either closing or not. Or in asynchronous lock case, where I think a reply will be sent later. > • vs_dispatch returns 0 > • When nfsd_cache_lookup returns RC_DROPIT (the RPC is already in progress, or the client has retransmitted too soon): the server is going to reply anyway, safe to drop Agreed. > • pc_func returns nfserr_dropit (NFSv2's JUKEBOX) I assume as in the NLM case that either behavior's OK; do we have a reason to prefer one or the other? > • RQ_DROPME is set (deferred requests?) Yes. So, drop and don't close. > • pc_encode is NULL - probably rare and inconsequential > • svc_authorise returns non-zero > • svcauth_gss_release returns a negative errno when integrity or privacy reply wrapping fails; i think this needs a connection reset > • incoming RPC header is shorter than 24 bytes - connection reset would be better here anyway, IMO Agreed. > The question I have is what does SVC_CLOSE mean for a UDP transport? Hm. svc_close_xprt->svc_delete_xprt-> xpo_detach->svc_sock_detach->release_sk() Uh, I don't know what exactly that does in the udp case. Anyway, I guess a pretty good rule is that we want to close whenever there's not a reply pending, so basically whenever we're not doing the deferred request thing. Might be simpler to make that logic explicit by returning one error in both cases and letting svc_process_common decide which case we're in by checking whether there's a deferred request. (Haven't checked how to do that.) --b. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: audit of the use of SVC_DROP in server reply path 2016-10-07 21:28 ` Bruce Fields @ 2016-10-13 13:50 ` Chuck Lever 0 siblings, 0 replies; 3+ messages in thread From: Chuck Lever @ 2016-10-13 13:50 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Linux NFS Mailing List > On Oct 7, 2016, at 5:28 PM, Bruce Fields <bfields@fieldses.org> wrote: > > Apologies for the delayed response: > > On Tue, Sep 13, 2016 at 11:42:37AM -0400, Chuck Lever wrote: >> I think the two interesting cases are svc_set_client and svc_authorise. >> >> Who does a "goto dropit;" inside svc_process_common? >> >> • svc_set_client returns SVC_DROP >> • When svcauth_gss_set_client calls svc_unix_set_client, which can return SVC_DROP >> • When svc_unix_set_client calls cache_check, and it returns -EAGAIN >> • When svc_unix_set_client calls unix_gid_find, and it returns -EAGAIN > > These should still result in a silent drop, since in these places we're > still planning to reply--only after deferring and then revisiting the > request later. (Though it might be worth verifying that there aren't > any weird subcases where that isn't the case.) > >> • svcauth_gss_accept >> • when gc_proc == RPC_GSS_PROC_DATA or RPC_GSS_PROC_DESTROY, and gss_check_seq_num fails > > This is the case that was causing us trouble. Should be a close. My original patch makes this change. >> • when gc_proc == RPC_GSS_PROC_DESTROY and the result length is larger than a page > > We're never going to reply, so close. Add a hunk to make a similar change here? >> • pc_func returns rpc_drop_reply - only used by NLM > > Looks like this might happen in some cases on open? (Delegation > conflict, or export lookup? I'm not sure.) In any case, this is NLM, > which I'm guessing can deal with us either closing or not. Or in > asynchronous lock case, where I think a reply will be sent later. > >> • vs_dispatch returns 0 >> • When nfsd_cache_lookup returns RC_DROPIT (the RPC is already in progress, or the client has retransmitted too soon): the server is going to reply anyway, safe to drop > > Agreed. > >> • pc_func returns nfserr_dropit (NFSv2's JUKEBOX) > > I assume as in the NLM case that either behavior's OK; do we have a > reason to prefer one or the other? > >> • RQ_DROPME is set (deferred requests?) > > Yes. So, drop and don't close. > >> • pc_encode is NULL - probably rare and inconsequential >> • svc_authorise returns non-zero >> • svcauth_gss_release returns a negative errno when integrity or privacy reply wrapping fails; i think this needs a connection reset Another hunk to change this case to SVC_CLOSE? >> • incoming RPC header is shorter than 24 bytes - connection reset would be better here anyway, IMO > > Agreed. And one more for this one? >> The question I have is what does SVC_CLOSE mean for a UDP transport? > > Hm. svc_close_xprt->svc_delete_xprt-> > xpo_detach->svc_sock_detach->release_sk() > > Uh, I don't know what exactly that does in the udp case. > > Anyway, I guess a pretty good rule is that we want to close whenever > there's not a reply pending, so basically whenever we're not doing the > deferred request thing. Might be simpler to make that logic explicit by > returning one error in both cases and letting svc_process_common decide > which case we're in by checking whether there's a deferred request. > (Haven't checked how to do that.) That seems like a more risky change. -- Chuck Lever ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-13 13:50 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-13 15:42 audit of the use of SVC_DROP in server reply path Chuck Lever 2016-10-07 21:28 ` Bruce Fields 2016-10-13 13:50 ` Chuck Lever
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.