So sorry for not catching this sooner. On Tue, Dec 19, 2006 at 08:07:54PM -0800, Erik Nordmark wrote: <mucho snippage deleted!>
<So far, so good, no additional replies needed...> > >DM-11 Line 1093-97, T4 Is disallowing command from rput a > >distinct > > etc. bugfix? At least file one if it is, please. > > From wput, but ... > We are fixing this as part of IP Instances since root in an exclusive-IP > zone could otherwise potentially cause a kernel panic by sending garbage > commands down to /dev/arp. Understood. > >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. > >usr/src/uts/common/inet/ip.h > > > >------- --------------- ------- > >----------------------------------------------- > >No. Location Sev. Comment > >------- --------------- ------- > >----------------------------------------------- > > > >DM-16 netstack ptrs T4 Some are held, some aren't. Maybe when > >I > > get to the new netstack code it'll become > > clear what the rules are. > > > > If it doesn't, then somewhere the why-held, > > why-not-held needs to be addressed. > > The design document tries to explain it. Can you check if that > explanation is sufficiently clear. See > http://www.opensolaris.org/os/project/crossbow/Docs/si-design.pdf > section 5.1 > > But you are correct that we should capture the general scheme in > netstack.h. Thanks for the netstack.h inclusion. > >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. > >usr/src/uts/common/inet/ip/ip.c > > > >------- --------------- ------- > >----------------------------------------------- > >No. Location Sev. Comment > >------- --------------- ------- > >----------------------------------------------- > > > >DM-18 ipsec_in_ns, T4 See DM-16. Why is not-holding > >acceptable? > > e.g. Line 2051 > > The overall explanation is at > http://www.opensolaris.org/os/project/crossbow/Docs/si-design.pdf > section 5.1 > > I can add some more to ipsec_info.h of this form: > * Some of these messages include pointers such as a netstack_t pointer. > * We do not explicitly reference count those with netstack_hold/rele, > * since we depend on IP's ability to discard all of the IPSEC_{IN,OUT} > * messages in order to handle the ipsa pointers. > * We have special logic when doing asynch callouts to kEF for which we > * verify netstack_t pointer using the netstackid_t. > > 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. > >usr/src/uts/common/inet/ip/sadb.c > > > >------- --------------- ------- > >----------------------------------------------- > >No. Location Sev. Comment > >------- --------------- ------- > >----------------------------------------------- > > > >DM-25 Lines 2099, T4 Please confirm my understanding: > >ALL_ZONES > > 2123 for the ire_ctable_lookup*() calls is one of > > the things you hide with IP instances, right? > > If I add an SA with an address of another > > non-shared IP instance, it'll be properly > > treated as non-local, right? > > "hide" in the sense that an exclusive-IP zone will never need to use > ALL_ZONES and zoneid == 0 in all of the TCP/IP code. > Thus inside an exclusive-IP zone the shared-IP logic is not used. > > When a SA is added by and for an exclusive-IP zone, it goes into a > different netstack_t. Thus from the perspective of an exclusive-IP zone > all the other zones on the machine are remote and not local. > > Did that answer your question? Yes it did. > >DM-30 Line 847 T5 INDEPENDENT BUG: That function > >shouldn't > > be called in ipsec_swap_policy(), it should > > be called in swap_global_policy(). There are > > other places it gets called where it probably > > shouldn't unless the policy head passed in is > > the systemwide one (vs. the policy head of a > > tunnel in an ipsec_tun_pol_t). > > > > If you have the cycles to fix it, that'd > > be great. Otherwise a bug against > > network/ipsec introduced in snv_53 would be > > in order. > > > > It's a p5, 'cause it's just an expensive NOP > > when the tunnel-path calls it. > > I've filed CR 6506488 Thank you. > >DM-30.5 Policy heads It affects YOUR project, though, because > > fixing the above p5 would reduce the number of > > calls (mostly dealing with policy heads) that > > need netstack_t as a parameter. Policy heads > > are independent objects that appear not only > > globally, but in tunnel instances too. > > > >DM-31 See DM-26, but more of it! > > Consider it done. Thanks. > >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. > >usr/src/uts/common/os/kmem.c > > > >------- --------------- ------- > >----------------------------------------------- > >No. Location Sev. Comment > >------- --------------- ------- > >----------------------------------------------- > > > >DM-38 General T5 Dumb question: why initialize here? > > Needs to be initialized before the first netstack_register which happens > when ip's _init() is called. And since the zones zsd callbacks are > initialized in the kmem code (earlier in the same function) I figured it > was a reasonable place. ACK. Thanks for the hard work! Dan _______________________________________________ networking-discuss mailing list [email protected]
