Dan McDonald wrote:
BTW: I ran into this CR IPsec CR once
6512601 panic in ipsec_in_tag - allocation failure
and I have a fix for it in spd.c in
file:///net/nptbld-x.sfbay/disk1/nordmark/si-only/webrev/index.html
If you review the fix then we can do it as part of the IP Instances putback.
DM-12 ar_open() T2 or What if netstack_find_by_cred() fails?
E2 Or won't it? A small comment would be nice
if it never fails.
It will never fail.
The ASSERT is partly there as documentation. The ASSERT is for
netstack_arp field.
Do you prefer two asserts as a way to document this e.g.
ns = netstack_find_by_cred();
ASSERT(ns != NULL);
as = ns->netstack_arp;
ASSERT(as != NULL);
Yeah, that works better for ASSERT()-as-documentation purposes.
OK. I'll tweak that.
usr/src/uts/common/inet/ip/icmp_opt_data.c
------- --------------- -------
-----------------------------------------------
No. Location Sev. Comment
------- --------------- -------
-----------------------------------------------
DM-17 Line 153 T4 This looks like a separate bugfix. Is it?
It showed up with IP Instances since we depend on the privilege checks
working. Do you want to have us file a separate CR?
Most people like to see distinct CRs when prior-to-project bugs are
found-and-fixed. E.g. Tunnel Reform found a bunch of flaky code due to its
test regimen.
I'm not sure whether it is "most" or a "few vocal ones".
It is useful to have a separate CR it is a bug that somebody else it
likely to run into (e.g., the CR for ipsec_in_tag), but if we make this
a "must" then it actually becomes an impediment to improving Solaris.
For instance, if you ask that the future IP datapath refactoring file a
CR for every bug it fixes then that project would be dead in its tracks;
while I can figure how things should work to be correct I can't actually
see all the current bugs that are in the interaction between e.g. IPv6
external resolvers, MULTIRT, IPsec, and multicast. We don't even know if
that combination works. Thus I don't think this should be a "must" and
there isn't a strong argument to having a separate CR in this case.
Note that I think IPsec in onnv has an issue with kEF asynch operations
where there doesn't seem to be a way to prevent esp/ah from unloading
with pending asynch callbacks. If you think this an issue in onnv I can
file a CR.
Currently ESP and AH *can't* unload once they're loaded, but please file a P5
bug if we add the ability to unload AH/ESP.
I think you mean that it can't be unloaded once keysock_plumb_ipsec()
has run; it does unload after having be loaded by the reconfiguration
reboot mechanism that loads everything (and I've seen it unload).
But if it doesn't load once it has been plumbed, why do the esp/ah
kstats use KSTAT_FLAG_PERSISTENT? I thought the motivation for that was
to handle not loosing the stats when there is an unload followed by a load.
DM-33 Action hash T4 I might argue that the interned action
pointers might be global across all
instances. IMHO it's a good question for
ipsec-core (esp. Bill).
The approach we've taking is "everything is separate unless it needs to
be shared" and opposed to "everything is shared unless it needs to be
separate". Thus it would take some convincing that the action hash must
be shared.
I think Bill mentioned something to you off-list. It's all about memory
footprint - we'll see if it becomes an issue if someone configures a box with
lotsa non-shared zones with similar IPsec policies.
That same type of argument can be applied to other parts of IP for the
cases when lots of IP Instances have the same configuration.
And sharing some data structures but not others lead to a complexity
explosion; part of the motivation for IP Instances is actually to allow
us to at least study getting rid of the implementation complexity
introduced by the shared stack zones support.
Erik
_______________________________________________
networking-discuss mailing list
[email protected]
