Merge lp:~matej-sekoranja/epics-base/ca-username-change into lp:~epics-core/epics-base/3.16

Proposed by Matej Sekoranja on 2015-01-05
Status: Rejected
Rejected by: Andrew Johnson on 2018-05-09
Proposed branch: lp:~matej-sekoranja/epics-base/ca-username-change
Merge into: lp:~epics-core/epics-base/3.16
Diff against target: 352 lines (+132/-34)
14 files modified
src/ca/client/access.cpp (+10/-3)
src/ca/client/ca_client_context.cpp (+6/-0)
src/ca/client/cac.cpp (+22/-0)
src/ca/client/cac.h (+1/-0)
src/ca/client/cacIO.h (+3/-0)
src/ca/client/cadef.h (+2/-1)
src/ca/client/nciu.h (+1/-1)
src/ca/client/oldAccess.h (+4/-0)
src/ca/client/virtualCircuit.h (+3/-2)
src/ca/legacy/pcas/generic/casStrmClient.cc (+7/-7)
src/ioc/db/dbCAC.h (+2/-0)
src/ioc/db/dbContext.cpp (+6/-0)
src/ioc/rsrv/camessage.c (+64/-19)
src/ioc/rsrv/server.h (+1/-1)
To merge this branch: bzr merge lp:~matej-sekoranja/epics-base/ca-username-change
Reviewer Review Type Date Requested Status
Andrew Johnson Disapprove on 2018-05-09
mdavidsaver 2015-01-05 Needs Fixing on 2015-08-18
Ralph Lange Needs Fixing on 2015-01-07
Review via email: mp+245612@code.launchpad.net

Description of the change

CAC: Implemented (deprecated) ca_modify_user_name() method.
rsrv: Accepts user name message even if there are active channels present and calls AS lib to update their access rights.

Increased CA minor version to 14.

No changes where done for CA portable channel access.

To post a comment you must log in.
Ralph Lange (ralph-lange) wrote :

Things I would like to see:

* A documented reason for this development. (Use case, spec, requirements ... something in that regime.)
* Update of the CA protocol documentation (if applicable).
* Update of Channel Access user documentation.
* Update of Base release notes.
* A test plan (probably using a test client) with expected results so regression can be tested against,
  at least manually.
  This should cover at least three setups:
  - old client, new server (trivial)
  - new client, old server
  - new client, new server

I do not like the idea of having CAS and rsrv supporting different versions of Channel Access. Depending on the motivation for this change, the CAS implementation might even be more important than rsrv.

mdavidsaver (mdavidsaver) wrote :

The RSRV code looks straightforward. I do have a couple of comments and questions.

I don't see that asChangeClient() is being used anywhere. The only mention I find is the cagateway, where it is ifdef'd out with a macro SUPPORT_OWNER_CHANGE. At first glance the implementation looks similar to asAddClient() so it may be OK.

Based on my experience with bug lp:1091401 testing need to include potential races with re-loading the ACF file with asInitAsyn().

Also, I haven't look in depth, but I notice that when is called asAddClient() in claim_ciu_action() no locks are held. Your change would call asChangeClient() with chanListLock held. Calls to asComputePvt() invoke callbacks. Do you have any thoughts on the potential for deadlocks?

review: Needs Information
Andrew Johnson (anj) wrote :

Before merging this I think we would need to have an equivalent implementation for the CAS server so the PV Gateway can be made to support the newer version of the CA protocol. That does pose one problem though – the CAS doesn't do variable length arrays yet, it still only supports protocol revision 12 and you can't add this support without doing that first/as well. Murali Shankar from SLAC was supposed to be working on that, but I don't know how far he's got with it so I've just written to him to ask.

I would also agree with Ralph's points that this will need the client API documenting in the caRef.html manual and in the protocol documentation (which I haven't merged into the AppDevGuide yet so hold off on doing that for now), and it will need an entry in the Release Notes too.

On the positive side, the implementation does look pretty much like I expected it would have to when I talked to Susan about it several months ago. You can continue to commit changes to your development branch and push them to launchpad (just 'bzr push', it should remember the branch URL). This merge proposal will be updated when you do that, but we won't be notified about such changes automatically, so add a comment to the merge proposal page when you want us to take another look.

Thanks,

- Andrew

review: Needs Fixing
Ralph Lange (ralph-lange) wrote :

Ah!
Susan is probably Suzanne (Gysin), and this is done to allow the role-based access stuff at ESS?

(Please, when asking for a merge, give a hint what the developments are actually good for or driven by, at least somewhere in a comment or commit message or blueprint or wiki page or mail!)

So, under the assumption that this is the intention of the development:
The heavily Gateway-based design for the ESS control system makes it *mandatory* that this feature is supported in CAS.
I would even say that rsrv support is useful for the development phase, but pretty pointless for production.

review: Needs Fixing
mdavidsaver (mdavidsaver) wrote :

> Based on my experience with bug lp:1091401 testing need to include potential
> races with re-loading the ACF file with asInitAsyn().

I've convinced myself that there is at least one potential deadlock here.

asInitAsyn() parses the new ACF file then triggers the asCaTask to call asComputeAllAsg() which locks the global asLock then (eventually) calls asComputePvt(). Calls to asComputePvt() may trigger some callbacks to casAccessRightsCB(), which locks chanListLock.

Your modified client_name_action locks chanListLock then calls asChangeClient(), which locks asLock and calls asComputePvt() as well.

So to order of locking between asLock and chanListLock is reversed in these two potentially concurrent paths.

So a deadlock is possible in the rather unfortunate situation that a CA client does a put which triggers asInitAsyn(), then quickly changes its client name at the same time as its write permission is to this record revoked (due to ACF policy changes).

review: Needs Fixing
Matej Sekoranja (matej-sekoranja) wrote :

> > Based on my experience with bug lp:1091401 testing need to include potential
> > races with re-loading the ACF file with asInitAsyn().
>
> I've convinced myself that there is at least one potential deadlock here.
>
> asInitAsyn() parses the new ACF file then triggers the asCaTask to call
> asComputeAllAsg() which locks the global asLock then (eventually) calls
> asComputePvt(). Calls to asComputePvt() may trigger some callbacks to
> casAccessRightsCB(), which locks chanListLock.
>
> Your modified client_name_action locks chanListLock then calls
> asChangeClient(), which locks asLock and calls asComputePvt() as well.
>
> So to order of locking between asLock and chanListLock is reversed in these
> two potentially concurrent paths.
>
> So a deadlock is possible in the rather unfortunate situation that a CA client
> does a put which triggers asInitAsyn(), then quickly changes its client name
> at the same time as its write permission is to this record revoked (due to ACF
> policy changes).

I would suggest to lock asLock before locking chanListLock. This establishes proper lock ordering.
The assumption is that the lock is recursive. Are locks in EPICS base recursive?

Andrew Johnson (anj) wrote :

Yes, an epicsMutex supports recursive locking.

Andrew Johnson (anj) wrote :

Minutes from a meeting with Vasu, Ralph, Michael and Andrew.

The server-side implementation of this also needs to be done in the CAS. This is a new API for servers, need to ensure that this doesn't add a security hole to existing servers.

We also need to audit this for security when talking to existing servers that don't support this version, and to old applications built with the new library that haven't implemented the new API.

Ralph says that Murali has a test framework for the gateway where we might be able to implement these kinds of audit tests.

Michael suggests that one possible response to unimplemented methods might be to drop the connection, forcing the client to reconnect with the new username.

This would go into 3.16 which is targeted for a December release, feature complete by early November.

12628. By Matej Sekoranja on 2015-06-30

Resolved possible user-name change deadlock and added pCAS owner change notification

mdavidsaver (mdavidsaver) wrote :

I'm commenting now because I've just noticed that there was activity on this branch. Since launchpad is sparing with email notifications it's a good idea to send a note to core-talk when pushing changes to a branch after a long period of inactivity.

review: Needs Fixing
mdavidsaver (mdavidsaver) wrote :

Rather than jumping through hoops with the channel lists, it seems simpler to fix the lock order issue by explicitly locking asLock around chanListLock in client_name_action.

Andrew Johnson (anj) wrote :

Are these changes still required? If ESS are hoping to go pvAccess only the answer may be no.

review: Needs Information
Matej Sekoranja (matej-sekoranja) wrote :

> Are these changes still required? If ESS are hoping to go pvAccess only the
> answer may be no.

True, pvAccess is the alternative.
Otherwise I would add this feature to CA to allow more advanced authorisation schemes.

Andrew Johnson (anj) wrote :

F2F 3/14/2017: implementation seems mostly complete, needs some fixes (race conditions, allocation failure handling, update CA_MINOR_PROTOCOL_REVISION value).
Need to ping ESS (Timo) to ask if this change is still needed/wanted.

Andrew Johnson (anj) wrote :

Core Meeting: Not required by ESS any more, could be revisited (if fixed) later, but rejecting for now.

review: Disapprove

Unmerged revisions

12628. By Matej Sekoranja on 2015-06-30

Resolved possible user-name change deadlock and added pCAS owner change notification

12627. By Matej Sekoranja on 2015-01-05

Implemented CAC ca_modify_user_name, and rsrv handling it

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ca/client/access.cpp'
2--- src/ca/client/access.cpp 2013-06-07 23:08:38 +0000
3+++ src/ca/client/access.cpp 2015-06-30 08:28:30 +0000
4@@ -230,11 +230,18 @@
5 //
6 // ca_modify_user_name()
7 //
8-// defunct
9-//
10 // extern "C"
11-int epicsShareAPI ca_modify_user_name ( const char * )
12+int epicsShareAPI ca_modify_user_name ( const char * pUserName )
13 {
14+ ca_client_context * pcac;
15+ int caStatus = fetchClientContext (&pcac);
16+ if ( caStatus != ECA_NORMAL ) {
17+ return caStatus;
18+ }
19+
20+ epicsGuard < epicsMutex > guard ( pcac->mutex );
21+ pcac->modifyUserName ( guard, pUserName );
22+
23 return ECA_NORMAL;
24 }
25
26
27=== modified file 'src/ca/client/ca_client_context.cpp'
28--- src/ca/client/ca_client_context.cpp 2014-02-07 20:13:12 +0000
29+++ src/ca/client/ca_client_context.cpp 2015-06-30 08:28:30 +0000
30@@ -668,6 +668,12 @@
31 guard, pChannelName, chan, pri );
32 }
33
34+void ca_client_context::modifyUserName (
35+ epicsGuard < epicsMutex > & guard, const char * pUserName )
36+{
37+ this->pServiceContext->modifyUserName ( guard, pUserName );
38+}
39+
40 void ca_client_context::flush ( epicsGuard < epicsMutex > & guard )
41 {
42 this->pServiceContext->flush ( guard );
43
44=== modified file 'src/ca/client/cac.cpp'
45--- src/ca/client/cac.cpp 2013-06-07 23:08:38 +0000
46+++ src/ca/client/cac.cpp 2015-06-30 08:28:30 +0000
47@@ -367,6 +367,28 @@
48 }
49
50 //
51+// update user name on all virtual circuits
52+//
53+void cac::modifyUserName (
54+ epicsGuard < epicsMutex > & guard,
55+ const char * pNewUserName )
56+{
57+ size_t len;
58+
59+ guard.assertIdenticalMutex ( this->mutex );
60+
61+ len = strlen ( pNewUserName ) + 1;
62+ this->pUserName = new char [ len ];
63+ strncpy ( this->pUserName, pNewUserName, len );
64+
65+ tsDLIter < tcpiiu > iter = this->circuitList.firstIter ();
66+ while ( iter.valid () ) {
67+ iter->userNameSetRequest ( guard );
68+ iter++;
69+ }
70+}
71+
72+//
73 // set the push pending flag on all virtual circuits
74 //
75 void cac::flush ( epicsGuard < epicsMutex > & guard )
76
77=== modified file 'src/ca/client/cac.h'
78--- src/ca/client/cac.h 2013-06-07 23:08:38 +0000
79+++ src/ca/client/cac.h 2015-06-30 08:28:30 +0000
80@@ -214,6 +214,7 @@
81 static unsigned lowestPriorityLevelAbove ( unsigned priority );
82 static unsigned highestPriorityLevelBelow ( unsigned priority );
83 void destroyIIU ( tcpiiu & iiu );
84+ void modifyUserName ( epicsGuard < epicsMutex > & guard, const char * pNewUserName );
85
86 const char * pLocalHostName ();
87
88
89=== modified file 'src/ca/client/cacIO.h'
90--- src/ca/client/cacIO.h 2013-06-07 23:08:38 +0000
91+++ src/ca/client/cacIO.h 2015-06-30 08:28:30 +0000
92@@ -286,6 +286,9 @@
93 cacChannel::priLev = cacChannel::priorityDefault ) = 0;
94 virtual void flush (
95 epicsGuard < epicsMutex > & ) = 0;
96+ virtual void modifyUserName (
97+ epicsGuard < epicsMutex > &,
98+ const char * ) = 0;
99 virtual unsigned circuitCount (
100 epicsGuard < epicsMutex > & ) const = 0;
101 virtual void selfTest (
102
103=== modified file 'src/ca/client/cadef.h'
104--- src/ca/client/cadef.h 2011-10-19 18:07:00 +0000
105+++ src/ca/client/cadef.h 2015-06-30 08:28:30 +0000
106@@ -887,10 +887,11 @@
107 void * pArg, ca_real p_delta, ca_real n_delta, ca_real timeout,
108 evid * pEventID, long mask );
109
110+epicsShareFunc int epicsShareAPI ca_modify_user_name ( const char *pUserName );
111+
112 /*
113 * defunct
114 */
115-epicsShareFunc int epicsShareAPI ca_modify_user_name ( const char *pUserName );
116 epicsShareFunc int epicsShareAPI ca_modify_host_name ( const char *pHostName );
117
118 #ifdef __cplusplus
119
120=== modified file 'src/ca/client/nciu.h'
121--- src/ca/client/nciu.h 2013-06-07 23:08:38 +0000
122+++ src/ca/client/nciu.h 2015-06-30 08:28:30 +0000
123@@ -41,7 +41,7 @@
124 # include "shareLib.h"
125 #endif
126
127-#define CA_MINOR_PROTOCOL_REVISION 13
128+#define CA_MINOR_PROTOCOL_REVISION 14
129 #include "caProto.h"
130
131 #include "cacIO.h"
132
133=== modified file 'src/ca/client/oldAccess.h'
134--- src/ca/client/oldAccess.h 2015-03-13 16:50:26 +0000
135+++ src/ca/client/oldAccess.h 2015-06-30 08:28:30 +0000
136@@ -303,6 +303,9 @@
137 cacChannel & createChannel (
138 epicsGuard < epicsMutex > &, const char * pChannelName,
139 cacChannelNotify &, cacChannel::priLev pri );
140+ void modifyUserName (
141+ epicsGuard < epicsMutex > &,
142+ const char * );
143 void flush ( epicsGuard < epicsMutex > & );
144 void eliminateExcessiveSendBacklog (
145 epicsGuard < epicsMutex > &, cacChannel & );
146@@ -369,6 +372,7 @@
147 long mask, caEventCallBackFunc * pCallBack, void * pCallBackArg,
148 evid *monixptr );
149 friend int epicsShareAPI ca_flush_io ();
150+ friend int epicsShareAPI ca_modify_user_name ( const char *pUserName );
151 friend int epicsShareAPI ca_clear_subscription ( evid pMon );
152 friend int epicsShareAPI ca_sg_create ( CA_SYNC_GID * pgid );
153 friend int epicsShareAPI ca_sg_delete ( const CA_SYNC_GID gid );
154
155=== modified file 'src/ca/client/virtualCircuit.h'
156--- src/ca/client/virtualCircuit.h 2013-06-07 23:08:38 +0000
157+++ src/ca/client/virtualCircuit.h 2015-06-30 08:28:30 +0000
158@@ -205,6 +205,9 @@
159 epicsPlacementDeleteOperator (( void *,
160 tsFreeList < class tcpiiu, 32, epicsMutexNOOP > & ))
161
162+ void userNameSetRequest (
163+ epicsGuard < epicsMutex > & );
164+
165 private:
166 hostNameCache hostNameCacheInstance;
167 tcpRecvThread recvThread;
168@@ -294,8 +297,6 @@
169 epicsGuard < epicsMutex > & );
170 void hostNameSetRequest (
171 epicsGuard < epicsMutex > & );
172- void userNameSetRequest (
173- epicsGuard < epicsMutex > & );
174 void createChannelRequest (
175 nciu &, epicsGuard < epicsMutex > & );
176 void writeRequest (
177
178=== modified file 'src/ca/legacy/pcas/generic/casStrmClient.cc'
179--- src/ca/legacy/pcas/generic/casStrmClient.cc 2014-05-30 17:36:50 +0000
180+++ src/ca/legacy/pcas/generic/casStrmClient.cc 2015-06-30 08:28:30 +0000
181@@ -1462,13 +1462,6 @@
182 char *pMalloc;
183 caStatus status;
184
185- // currently this has to occur prior to
186- // creating channels or its not allowed
187- if ( this->chanList.count () ) {
188- return this->sendErr ( guard, mp, invalidResID,
189- ECA_UNAVAILINSERV, pName );
190- }
191-
192 size = strlen(pName)+1;
193
194 /*
195@@ -1491,6 +1484,13 @@
196 }
197 this->pUserName = pMalloc;
198
199+ tsDLIter < casChannelI > iter =
200+ this->chanList.firstIter ();
201+ while ( iter.valid() ) {
202+ iter->setOwner( this->pUserName, this->pHostName );
203+ iter++;
204+ }
205+
206 return S_cas_success;
207 }
208
209
210=== modified file 'src/ioc/db/dbCAC.h'
211--- src/ioc/db/dbCAC.h 2015-04-09 20:53:16 +0000
212+++ src/ioc/db/dbCAC.h 2015-06-30 08:28:30 +0000
213@@ -212,6 +212,8 @@
214 cacChannel::priLev );
215 void flush (
216 epicsGuard < epicsMutex > & );
217+ void modifyUserName (
218+ epicsGuard < epicsMutex > & guard, const char * );
219 unsigned circuitCount (
220 epicsGuard < epicsMutex > & ) const;
221 void selfTest (
222
223=== modified file 'src/ioc/db/dbContext.cpp'
224--- src/ioc/db/dbContext.cpp 2013-06-07 23:08:38 +0000
225+++ src/ioc/db/dbContext.cpp 2015-06-30 08:28:30 +0000
226@@ -389,6 +389,12 @@
227 }
228 }
229
230+void dbContext::modifyUserName (
231+ epicsGuard < epicsMutex > & guard, const char * )
232+{
233+ // noop
234+}
235+
236 unsigned dbContext::circuitCount (
237 epicsGuard < epicsMutex > & guard ) const
238 {
239
240=== modified file 'src/ioc/rsrv/camessage.c'
241--- src/ioc/rsrv/camessage.c 2015-04-22 21:51:31 +0000
242+++ src/ioc/rsrv/camessage.c 2015-06-30 08:28:30 +0000
243@@ -965,25 +965,11 @@
244 size_t size;
245 char *pName;
246 char *pMalloc;
247- int chanCount;
248-
249- epicsMutexMustLock ( client->chanListLock );
250- chanCount =
251- ellCount ( &client->chanList ) +
252- ellCount ( &client->chanPendingUpdateARList );
253- epicsMutexUnlock( client->chanListLock );
254-
255- if ( chanCount != 0 ) {
256- SEND_LOCK ( client );
257- send_err(
258- mp,
259- ECA_INTERNAL,
260- client,
261- "attempts to use protocol to set user name "
262- "after creating first channel ignored by server" );
263- SEND_UNLOCK ( client );
264- return RSRV_OK;
265- }
266+ int doUpdateList = 1;
267+ int status;
268+ struct channel_in_use *pciu;
269+ size_t i, count;
270+ struct channel_in_use **pciu_list_copy;
271
272 pName = (char *) pPayload;
273 size = strlen(pName)+1;
274@@ -1028,6 +1014,65 @@
275 free ( pName );
276 }
277
278+ epicsMutexMustLock ( client->chanListLock );
279+
280+ count = ellCount ( &client->chanList ) +
281+ ellCount ( &client->chanPendingUpdateARList );
282+ pciu_list_copy =
283+ ( struct channel_in_use ** ) calloc(count, sizeof(struct channel_in_use *));
284+
285+ /*
286+ * First we do a copy of a channels list holding a lock,
287+ * and then we call asChangeClient without locking.
288+ * Since this method is called by TCP read thread itself
289+ * channels cannot be deleted during the call of this method.
290+ */
291+ pciu = ( struct channel_in_use * )
292+ ellFirst ( & client->chanPendingUpdateARList );
293+ i = 0;
294+ while(1) {
295+
296+ if(!pciu) {
297+ if(doUpdateList) {
298+ pciu = ( struct channel_in_use * )
299+ ellFirst( & client->chanList );
300+ if (!pciu)
301+ break;
302+ doUpdateList = 0;
303+ }
304+ else
305+ break;
306+ }
307+
308+ pciu_list_copy[i++] = pciu;
309+
310+ pciu = ( struct channel_in_use * )
311+ ellNext ( & pciu->node );
312+ }
313+
314+ epicsMutexUnlock( client->chanListLock );
315+
316+ for ( i = 0; i < count; i++ ) {
317+ /*
318+ * update access security for this channel
319+ */
320+ status = asChangeClient(
321+ pciu_list_copy[i]->asClientPVT,
322+ asDbGetAsl(pciu_list_copy[i]->dbch),
323+ client->pUserName ? client->pUserName : "",
324+ client->pHostName ? client->pHostName : "");
325+ if(status != 0 && status != S_asLib_asNotActive){
326+ log_header ("No room for security table",
327+ client, mp, pPayload, 0);
328+ SEND_LOCK(client);
329+ send_err(mp, ECA_ALLOCMEM, client, "No room for security table");
330+ SEND_UNLOCK(client);
331+ }
332+ }
333+
334+ free(pciu_list_copy);
335+
336+
337 return RSRV_OK;
338 }
339
340
341=== modified file 'src/ioc/rsrv/server.h'
342--- src/ioc/rsrv/server.h 2014-03-06 21:55:13 +0000
343+++ src/ioc/rsrv/server.h 2015-06-30 08:28:30 +0000
344@@ -27,7 +27,7 @@
345 #include "asLib.h"
346 #include "dbChannel.h"
347 #include "dbNotify.h"
348-#define CA_MINOR_PROTOCOL_REVISION 13
349+#define CA_MINOR_PROTOCOL_REVISION 14
350 #include "caProto.h"
351 #include "ellLib.h"
352 #include "epicsTime.h"

Subscribers

People subscribed via source and target branches