610843 - Need way to recover softoken in child after fork()

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Robert Relyea]

With more libraries using NSS, sometimes the application calling the library may fork(). This, of course, causes known problems with NSS because of PKCS #11 library semantics.

We need a way to recover from this without tearing down all of NSS. I simple function can reinitialize all the PKCS #11 modules at some time after a fork() and allow NSS to continue to work. After this reinitialization, old objects will no longer function (but also won't crash). New objects will work just fine.

Tokens may get calls with invalid handles after such a call. Tokens are required to deal with them (by returning an error) by the PKCS #11 spec.

Comment 1

[Robert Relyea]

Attachment: Restart any NSS Modules if necessary

SECMOD_RestartModules() can be called after a fork(). It will walk down the list of initialized modules, and check to see if they are still functioning. If not, it will Finalize and restart them. If the force variable is set to PR_TRUE, then it will Finalize and restart all modules on the initialized list.

If you call this function with (PR_FALSE) and the process hasn't forked, it will be a noop (as all modules should be functioning normally). If you call it with PR_TRUE, however, you will force all modules to reinitialize, losing your keys and login state for your whole process.

bob

Comment 2

[Robert Relyea]

Comment on attachment 489353 [details] [diff] [review]
Restart any NSS Modules if necessary

This one is shorter, wan-teh;).

bob

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Just to be clear, by "after fork", you mean "in the child", right?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 489353 [details] [diff] [review]
Restart any NSS Modules if necessary

Bob, this is a weird diff.  How about a diff -u?

Comment 5

[Robert Relyea]

> Just to be clear, by "after fork", you mean "in the child", right?

yes

> Bob, this is a weird diff.  How about a diff -u?

*bob* bops his head. oops, now diff coming up..

Comment 6

[Robert Relyea]

Attachment: Restart any NSS Modules if necessary (-u version)

patch with the -u

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 489376 [details] [diff] [review]
Restart any NSS Modules if necessary (-u version)

Bob, I have some review comments, some questions, 
and some suggested changes to the block comment, which are 
worth doing but not worth an r-.  
First the trivial stuff.

You have a large block comment that appears in two places.  
It has the same problems in both places. 
I'll only show one copy.

>+ * after a fork(), PKCS #11 says we need to call C_Initialize again in 
>+ * the child before we can use it. This function causes this reinitialization.

In the next line, I think the word "there" can just be removed, and 
the word "you" should be "your".

>+ * NOTE: there any outstanding handles will become invalid, which means you
>+ * keys and contexts will fail, but new ones can be created.
>+ *

In the next line, you mention "the shutdown", but until here, you've said
this does a reinitialization.  I think this sentence should also describe
the operation as a reinitialization.  

>+ * force means to do the shutdown even if the PKCS #11 module does not seem

In the next sentence, s/forke/fork/ and s/accross/across/

>+ * to need it. This allows software modules which ignore forke to preserve 
>+ * their keys accross the fork().
>+ */
>+SECStatus
>+SECMOD_RestartModules(PRBool force)
>+{
>+    SECMODModuleList *mlp;
>+    SECStatus rrv = SECSuccess;
>+    int lastError = 0;
>+
>+    if (!moduleLock) {
>+    	PORT_SetError(SEC_ERROR_NOT_INITIALIZED);
>+	return SECFailure;
>+    }

Above, this function fails if nss is not initialized.  
Should it perhaps report success instead?
If NSS had not been initialized in the parent at the time of fork,
then that's a virtually ideal condition in the child.  It doesn't
seem like a failure.  It's not really any different from finding
that no modules need reinitialization.
I don't know what the right answer is here. 

>+	    /* now initialize the module, this function reinitializes
>+	     * a module in place, preserving existing slots (even if they
>+	     * no longer exist) */
>+	    rv = secmod_ModuleInit(mod, NULL, &alreadyLoaded);
>+	    if (rv != SECSuccess) {
>+		/* save the last error code */
>+		lastError = PORT_GetError();
>+		rrv = rv;
>+	    }

here, if the ModuleInit call above fails, we nonetheless go ahead
and plow into the InitToken calls below.  
Should there be a continue statement in the block above?

>+	    for (i=0; i < mod->slotCount; i++) {
>+		/* get new token sessions, bump the series up so that
>+		 * we refresh other old sessions. This will tell much of
>+		 * NSS to flush cached handles it may hold as well */
>+		PK11_InitToken(mod->slots[i],PR_TRUE);

There's no record kept of any results.  Suppose some tokens succeed 
but others fail.  Or suppose ALL fail.  The caller never knows. 
I guess the caller has to be optimistic, and hope things don't 
fall apart after this.

>+	    }
>+	}
>+    }
>+    SECMOD_ReleaseReadLock(moduleLock);
>+
>+    if (rrv != SECSuccess) {
>+	/* restore the last error code */
>+	PORT_SetError(lastError);
>+    }
>+
>+    return rrv;

So if any module fails to re-init, the caller gets a failure result 
and an error code, but no knowledge of which module failed.  
Maybe this isn't substantially different than NSS_Init.  
I'm not giving any r- here.  But maybe a little discuss of the 
subject is in order (in this bug).

Comment 8

[Robert Relyea]

Nelson: Thanks for the quick review!

I'll go ahead and make all your described changes to the comment...

> here, if the ModuleInit call above fails, we nonetheless go ahead
> and plow into the InitToken calls below.  
> Should there be a continue statement in the block above?

I'm of two minds. Initially I thought that you were correct and we shouldn't plow on, but thinking about your last comment maybe we should (I'll explain below).

> There's no record kept of any results.

yeah, that's a bug, we should roll up the rv here.

> So if any module fails to re-init, the caller gets a failure result 
> and an error code, but no knowledge of which module failed. 

All the modules are still on the 'active modules list' so the application can walk down them and see which are alive, except I don't think that there is a single way for them to know if a module is 'alive' or not (short of our test to see if it responds to 'C_GetSlotCount', which I don't think the application has access to). OTOH, the application is probably more interested in whether or not individual slots are active (since that is what the application typically deals with). PK11_InitToken will mark improperly enabled slots as disabled, so the application can find all the slots that are disabled.

This comes back to your initial question about whether to plow ahead if we failed to initialize the module. In that case PK11_InitToken is pretty much guaranteed to fail, and the token would be disabled... but it might be cleaner and more efficient to just disable all the slots on a failed module.

In light of your review I think I would propose the following changes:

1) if the module fails to initialize, mark all the slots on the module as disabled.
2) if PK11_InitToken failes, set rrv to SECFailure and save the lastError.

bob

Comment 9

[Robert Relyea]

Attachment: V2: Restart any NSS Modules if necessary

Incorporate Nelson's feedback

Comment 10

[Wan-Teh Chang]

Comment on attachment 490237 [details] [diff] [review]
V2: Restart any NSS Modules if necessary

r=wtc.  Just some nits.

1. Should the new function be renamed SECMOD_RestartModules
to indicate that it should be called after fork()?  Or do
you want to keep it general and allow it to be called without
fork()?

2. Please keep the block comment just in the header secmod.h.
No need to duplicate it in two files.

3. In security/nss/lib/pk11wrap/pk11util.c:

>@@ -785,6 +785,7 @@ SECMODModuleList *SECMOD_NewModuleListEl
>     return newModList;
> }
> 
>+
> /*
>  * make a new reference to a module so It doesn't go away on us
>  */

It doesn't seem necessary to add this blank line.

>+    /* Only need to do the PKCS #11 modules that were initialized */

Change 'do' to 'restart', 'reset', or 'reinit'.

>+    for(mlp = modules; mlp != NULL; mlp = mlp->next) {

Add a space after 'for'.

>+	if (force  || (PK11_GETTAB(mod)->
>+			C_GetSlotList(PR_FALSE, NULL, &count) != CKR_OK)) {

Change PR_FALSE to CK_FALSE.

>+		/* couldn't reinit the module, disable all it's slots */

it's => its

4. In security/nss/lib/pk11wrap/secmod.h:

>+ * After a fork(), PKCS #11 says we need to call C_Initialize again in
>+ * the child before we can use it. This function causes this reinitialization.

it => a module

Comment 11

[Robert Relyea]

NSS 3.12 branch:
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.206.2.2; previous revision: 1.206.2.1
done
Checking in lib/pk11wrap/pk11util.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v  <--  pk11util.c
new revision: 1.58.2.1; previous revision: 1.58
done
Checking in lib/pk11wrap/secmod.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmod.h,v  <--  secmod.h
new revision: 1.27.2.1; previous revision: 1.27
done

NSS Trunk/Tip

/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.209; previous revision: 1.208
done
Checking in pk11wrap/pk11util.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v  <--  pk11util.c
new revision: 1.59; previous revision: 1.58
done
Checking in pk11wrap/secmod.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmod.h,v  <--  secmod.h
new revision: 1.28; previous revision: 1.27
done