Anthony Scarpino wrote:
> Darren J Moffat wrote:
>> Garrett D'Amore wrote:
>>> Darren J Moffat wrote:
>>>> It was on my suggestion that Tony use the kernel sys_shutdown
>>>> variable because I've seen several other kernel/userland door
>>>> services use this same variable to determine that they shouldn't
>>>> fail because the system is shutting down. So I think there is
>>>> plenty of sufficient, recently created, precedence for using this
>>>> method. Also given they are in the same consolidation I don't see
>>>> an issue since sys_shutdown is consolidation private (unless you
>>>> can point me to an ARC case that says it is project private in
>>>> which case we will seek a contract to use it).
>>>>
>>>>
>>>> kcfd has two jobs and yes given the new system duty cycle
>>>> scheduling class that part of kcfd's work could probably be done
>>>> that way - however nothing in that part of kcfd is what motivates
>>>> this change. However the other part of kcfd needs to remain in
>>>> userland because we have no intention of pushing certificate
>>>> parsing code down into the kernel.
>>>
>>> Fair enough. I still think the sys_shutdown check and corresponding
>>> messaging should be made within the framework though, and not
>>> scattered through each of the different modules.
>>
>> Agreed on that part it should be a framework issue hidden behind
>> crypto_register_provider and crypto_unregister_provider.
>>
>
> I had thought about going down this route, but backed away because it
> was more invasive and caused changes to the error message. In
> particular, some of the error messages would have to use the provider
> description and not the module name..
>
> So errors that use to look like:
>
> blowfish: [ID 733954 kern.warning] WARNING: blowfish did not register
> with the cryptographic framework
>
> would have to look like:
>
> kcf: [ID 733954 kern.warning] WARNING: Blowfish Kernel SW Provider did
> not register with the cryptographic framework
>
> If there is no problem seeing the provider description rather than the
> module name, then I don't mind changing that..
It seems like not a terrible thing. I'd actually like a more
descriptive error message too... *why* didn't the thing register? It
isn't because it didn't *try*, which is what the message implies.
For example: "WARNING: Unable to verify signature for Blowfish Kernel
SW Provider" is a better error message. (Fill in whatever reasons are
most appropriate.) The benefit of doing the messaging in the framework
is you *have* this information, whereas the crypto module has no idea
since it just gets a generic DDI_FAILURE type of return.
I'm also a bit surprised that these modules only register a fully
detailed description, and don't include a module or driver name (and
instance number if appropriate). Anytime a common framework needs to
message on behalf of a consuming module (think SCSA, GLDv3, or the
Boomer audio framework), we generally have that information so we can
cobble up an appropriate message. Then the message appears to come from
genunix, but looks like this:
unix: [ID XXXX kern.warning] WARNING: audio810#0: Could not allocate
engine structure
(or somesuch)
This smells like an opportunity for future improvement in the
framework. Given that the crypto provider interface is Consolidation
Only (it is, isn't it?), it seems like we could -- and should -- make
that change sometime soon... but possibly not until after b135.
So as a compromise, I'm willing to hold my nose and bless your changes
as written, *if* you'll also file a CR (not less than P4, ideally P3)
and agree change the messaging and handling as I've suggested, in the
next couple of open builds (say 135 or 136) after the restrictions on
the gate are lifted.
- Garrett
>
> Tony
