Sorry for not including the opensolaris list on this the first time. The good news is that I remembered less than 24 hours after I sent the note to Erik and friends. My apologies, Dan ===================== (Cut up to and including here.) ==================== Reviewer Name: Dan McDonald Document/Module Title: IP Instances Document/Module Author: Erik Nordmark, Yukun Zhang, Dong-Hai Han Document/Module Version/Date: 3 December 2006 Reviewer Preparation Time: 10+ hours usr/src/Makefile.master usr/src/cmd/diff/Makefile usr/src/cmd/mdb/common/modules/genunix/genunix.c usr/src/cmd/mdb/common/modules/genunix/net.c usr/src/cmd/mdb/common/modules/genunix/net.h usr/src/cmd/mdb/common/modules/genunix/netstack.c usr/src/cmd/mdb/common/modules/genunix/netstack.h usr/src/cmd/mdb/common/modules/hook/hook.c usr/src/cmd/mdb/common/modules/ip/ip.c usr/src/cmd/mdb/intel/amd64/genunix/Makefile usr/src/cmd/mdb/intel/ia32/genunix/Makefile usr/src/cmd/mdb/sparc/v9/genunix/Makefile usr/src/cmd/svc/milestone/net-init usr/src/cmd/svc/milestone/net-loopback usr/src/cmd/svc/milestone/net-physical usr/src/cmd/svc/milestone/net-svc usr/src/cmd/svc/shell/smf_include.sh usr/src/common/net/patricia/radix.c usr/src/lib/libc/port/mapfile-vers usr/src/lib/libc/port/sys/zone.c usr/src/pkgdefs/SUNWcnetr/pkginfo.tmpl usr/src/pkgdefs/SUNWhea/prototype_com usr/src/pkgdefs/common_files/i.devpolicy usr/src/pkgdefs/etc/exception_list_i386 usr/src/pkgdefs/etc/exception_list_sparc usr/src/uts/Makefile.uts usr/src/uts/common/Makefile.files usr/src/uts/common/inet/Makefile usr/src/uts/common/inet/arp.h usr/src/uts/common/net/radix.h usr/src/uts/common/inet/arp/arp_netinfo.c usr/src/uts/common/inet/inetddi.c usr/src/uts/common/inet/ip/icmp.c usr/src/uts/common/inet/ip/ip_sadb.c usr/src/uts/common/inet/ip/ip_srcid.c usr/src/uts/common/inet/ip/ipclassifier.c usr/src/uts/common/inet/ip/ipdrop.c usr/src/uts/common/inet/ip/rts.c usr/src/uts/common/inet/ip/rtsddi.c usr/src/uts/common/os/policy.c usr/src/uts/common/os/priv_defs usr/src/uts/common/sys/policy.h usr/src/uts/common/inet/ip/keysock.c usr/src/uts/common/inet/ip6.h usr/src/uts/common/inet/ip6_asp.h usr/src/uts/common/inet/ip_ftable.h usr/src/uts/common/inet/ip_if.h usr/src/uts/common/inet/ip_impl.h usr/src/uts/common/inet/ip_ire.h usr/src/uts/common/inet/ip_multi.h usr/src/uts/common/inet/ip_ndp.h usr/src/uts/common/inet/ip_netinfo.h usr/src/uts/common/inet/ip_rts.h usr/src/uts/common/inet/ip_stack.h usr/src/uts/common/inet/ipclassifier.h usr/src/uts/common/inet/ipdrop.h usr/src/uts/common/inet/ipp_common.h usr/src/uts/common/inet/ipsec_info.h usr/src/uts/common/inet/ipsecah.h usr/src/uts/common/inet/ipsecesp.h usr/src/uts/common/inet/keysock.h usr/src/uts/common/inet/optcom.h usr/src/uts/common/inet/rawip_impl.h usr/src/uts/common/inet/spdsock.h usr/src/uts/common/inet/tun.h usr/src/uts/common/sys/netstack.h usr/src/uts/common/sys/Makefile ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-0 Looks good, no comments needed. usr/src/cmd/ipf/etc/Makefile usr/src/cmd/ipf/lib/Makefile.com usr/src/cmd/ipf/lib/common/getifname.c usr/src/cmd/ipf/lib/common/getnattype.c usr/src/cmd/ipf/lib/common/getsumd.c usr/src/cmd/ipf/lib/common/load_hash.c usr/src/cmd/ipf/lib/common/load_pool.c usr/src/cmd/ipf/lib/common/printactivenat.c usr/src/cmd/ipf/lib/common/printhash.c usr/src/cmd/ipf/lib/common/printhash_live.c usr/src/cmd/ipf/lib/common/printhashdata.c usr/src/cmd/ipf/lib/common/printnat.c usr/src/cmd/ipf/lib/common/printpool.c usr/src/cmd/ipf/lib/common/printpool_live.c usr/src/cmd/ipf/lib/common/printpooldata.c usr/src/cmd/ipf/svc/Makefile usr/src/cmd/ipf/svc/ipfilter usr/src/cmd/ipf/svc/ipfilter.xml usr/src/cmd/ipf/tools/Makefile.tools usr/src/cmd/ipf/tools/ip_fil.c usr/src/cmd/ipf/tools/ipf.c usr/src/cmd/ipf/tools/ipf_y.y usr/src/cmd/ipf/tools/ipfstat.c usr/src/cmd/ipf/tools/ipftest.c usr/src/cmd/ipf/tools/ipnat.c usr/src/cmd/ipf/tools/ippool.c usr/src/cmd/mdb/common/modules/neti/neti.c usr/src/cmd/mdb/common/modules/sctp/sctp.c usr/src/cmd/zoneadm/zoneadm.c usr/src/cmd/zoneadmd/Makefile usr/src/cmd/zoneadmd/vplat.c usr/src/cmd/zonecfg/zonecfg.c usr/src/cmd/zonecfg/zonecfg.h usr/src/cmd/zonecfg/zonecfg_grammar.y usr/src/cmd/zonecfg/zonecfg_lex.l usr/src/cmd/zonename/zonename.c usr/src/head/libzonecfg.h usr/src/head/zone.h usr/src/lib/brand/native/zone/platform.xml usr/src/lib/brand/sn1/zone/platform.xml usr/src/lib/libbrand/common/libbrand.c usr/src/lib/libbrand/common/libbrand.h usr/src/lib/libbrand/dtd/zone_platform.dtd.1 usr/src/lib/libdladm/common/libdladm.c usr/src/lib/libdladm/common/libdladm.h usr/src/lib/libdladm/common/mapfile-vers usr/src/lib/libzonecfg/common/libzonecfg.c usr/src/lib/libzonecfg/common/mapfile-vers usr/src/lib/libzonecfg/dtd/zonecfg.dtd.1 usr/src/pkgdefs/SUNWipfh/prototype_com usr/src/pkgdefs/SUNWipfr/pkginfo.tmpl usr/src/uts/common/inet/ipf/fil.c usr/src/uts/common/inet/ipf/ip_auth.c usr/src/uts/common/inet/ipf/ip_fil_solaris.c usr/src/uts/common/inet/ipf/ip_frag.c usr/src/uts/common/inet/ipf/ip_htable.c usr/src/uts/common/inet/ipf/ip_log.c usr/src/uts/common/inet/ipf/ip_lookup.c usr/src/uts/common/inet/ipf/ip_nat.c usr/src/uts/common/inet/ipf/ip_pool.c usr/src/uts/common/inet/ipf/ip_proxy.c usr/src/uts/common/inet/ipf/ip_state.c usr/src/uts/common/inet/ipf/ipf.h usr/src/uts/common/inet/ipf/misc.c usr/src/uts/common/inet/ipf/netinet/Makefile usr/src/uts/common/inet/ipf/netinet/ip_auth.h usr/src/uts/common/inet/ipf/netinet/ip_compat.h usr/src/uts/common/inet/ipf/netinet/ip_fil.h usr/src/uts/common/inet/ipf/netinet/ip_frag.h usr/src/uts/common/inet/ipf/netinet/ip_ftp_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_h323_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_htable.h usr/src/uts/common/inet/ipf/netinet/ip_ipsec_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_irc_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_lookup.h usr/src/uts/common/inet/ipf/netinet/ip_nat.h usr/src/uts/common/inet/ipf/netinet/ip_netbios_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_pool.h usr/src/uts/common/inet/ipf/netinet/ip_pptp_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_proxy.h usr/src/uts/common/inet/ipf/netinet/ip_raudio_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_rcmd_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_rpcb_pxy.c usr/src/uts/common/inet/ipf/netinet/ip_state.h usr/src/uts/common/inet/ipf/netinet/ipf_stack.h usr/src/uts/common/inet/ipf/solaris.c usr/src/uts/common/inet/sctp/sctp.c usr/src/uts/common/inet/sctp/sctp_addr.c usr/src/uts/common/inet/sctp/sctp_addr.h usr/src/uts/common/inet/sctp/sctp_asconf.c usr/src/uts/common/inet/sctp/sctp_bind.c usr/src/uts/common/inet/sctp/sctp_common.c usr/src/uts/common/inet/sctp/sctp_conn.c usr/src/uts/common/inet/sctp/sctp_cookie.c usr/src/uts/common/inet/sctp/sctp_error.c usr/src/uts/common/inet/sctp/sctp_hash.c usr/src/uts/common/inet/sctp/sctp_heartbeat.c usr/src/uts/common/inet/sctp/sctp_impl.h usr/src/uts/common/inet/sctp/sctp_init.c usr/src/uts/common/inet/sctp/sctp_input.c usr/src/uts/common/inet/sctp/sctp_ioc.c usr/src/uts/common/inet/sctp/sctp_opt_data.c usr/src/uts/common/inet/sctp/sctp_output.c usr/src/uts/common/inet/sctp/sctp_param.c usr/src/uts/common/inet/sctp/sctp_shutdown.c usr/src/uts/common/inet/sctp/sctp_snmp.c usr/src/uts/common/inet/sctp/sctp_stack.h usr/src/uts/common/inet/sctp/sctp_timer.c usr/src/uts/common/inet/sctp_ip.h usr/src/uts/common/io/aggr/aggr_send.c usr/src/uts/common/io/dld/dld_drv.c usr/src/uts/common/io/dld/dld_str.c usr/src/uts/common/io/dls/dls.c usr/src/uts/common/io/dls/dls_vlan.c usr/src/uts/common/io/hook.c usr/src/uts/common/io/mac/mac.c usr/src/uts/common/io/neti.c usr/src/uts/common/io/sad.c usr/src/uts/common/io/sad_conf.c usr/src/uts/common/io/strplumb.c usr/src/uts/common/ipp/dlcosmk/dlcosmk.c usr/src/uts/common/ipp/ipgpc/classifierddi.c usr/src/uts/common/inet/ip/tn_ipopt.c usr/src/uts/common/inet/ip/tnet.c usr/src/uts/common/sys/sad.h usr/src/uts/common/sys/strsubr.h usr/src/uts/common/sys/syscall.h usr/src/uts/common/sys/tsol/tnet.h usr/src/uts/common/sys/zone.h usr/src/uts/common/inet/nca/nca.h usr/src/uts/common/inet/squeue.c usr/src/uts/common/os/space.c usr/src/uts/common/os/streamio.c usr/src/uts/common/os/strsubr.c usr/src/uts/common/os/zone.c usr/src/uts/common/sys/dld.h usr/src/uts/common/sys/dls.h usr/src/uts/common/sys/dls_impl.h usr/src/uts/common/sys/hook.h usr/src/uts/common/sys/hook_event.h usr/src/uts/common/sys/hook_impl.h usr/src/uts/common/sys/mac.h usr/src/uts/common/sys/neti.h ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-1 Do not feel qualified to inspect this file. usr/src/uts/common/inet/ip/igmp.c usr/src/uts/common/inet/ip/ip6.c usr/src/uts/common/inet/ip/ip6_asp.c usr/src/uts/common/inet/ip/ip6_if.c usr/src/uts/common/inet/ip/ip6_ire.c usr/src/uts/common/inet/ip/ip6_rts.c usr/src/uts/common/inet/ip/ip_ftable.c usr/src/uts/common/inet/ip/ip_if.c usr/src/uts/common/inet/ip/ip_ire.c usr/src/uts/common/inet/ip/ip_mroute.c usr/src/uts/common/inet/ip/ip_multi.c usr/src/uts/common/inet/ip/ip_ndp.c usr/src/uts/common/inet/ip/ip_netinfo.c usr/src/uts/common/inet/ip/ip_opt_data.c usr/src/uts/common/inet/ip/ip_rts.c usr/src/uts/intel/arp/Makefile usr/src/uts/intel/arp/arp.objt-symbols.obj64 usr/src/uts/intel/hook/Makefile usr/src/uts/intel/hook/hook.objt-symbols.obj64 usr/src/uts/intel/icmp/Makefile usr/src/uts/intel/icmp/icmp.objt-symbols.obj64 usr/src/uts/intel/ip/Makefile usr/src/uts/intel/ip/ip.objt-symbols.debug64 usr/src/uts/intel/ip/ip.objt-symbols.obj64 usr/src/uts/intel/ipf/Makefile usr/src/uts/intel/ipf/ipf.objt-symbols.obj64 usr/src/uts/intel/ipsecah/Makefile usr/src/uts/intel/ipsecah/ipsecah.objt-symbols.obj64 usr/src/uts/intel/ipsecesp/Makefile usr/src/uts/intel/ipsecesp/ipsecesp.objt-symbols.obj64 usr/src/uts/intel/keysock/Makefile usr/src/uts/intel/keysock/keysock.objt-symbols.obj64 usr/src/uts/intel/neti/Makefile usr/src/uts/intel/neti/neti.objt-symbols.obj64 usr/src/uts/intel/os/device_policy usr/src/uts/intel/rts/Makefile usr/src/uts/intel/rts/rts.objt-symbols.obj64 usr/src/uts/intel/spdsock/Makefile usr/src/uts/intel/spdsock/spdsock.objt-symbols.obj64 usr/src/uts/sparc/arp/Makefile usr/src/uts/sparc/arp/arp.objt-symbols.obj64 usr/src/uts/sparc/hook/Makefile usr/src/uts/sparc/hook/hook.objt-symbols.obj64 usr/src/uts/sparc/icmp/Makefile usr/src/uts/sparc/icmp/icmp.objt-symbols.obj64 usr/src/uts/sparc/ip/Makefile usr/src/uts/sparc/ip/ip.objt-symbols.debug64 usr/src/uts/sparc/ip/ip.objt-symbols.obj64 usr/src/uts/sparc/ipf/Makefile usr/src/uts/sparc/ipf/ipf.objt-symbols.obj64 usr/src/uts/sparc/ipsecah/Makefile usr/src/uts/sparc/ipsecah/ipsecah.objt-symbols.obj64 usr/src/uts/sparc/ipsecesp/Makefile usr/src/uts/sparc/ipsecesp/ipsecesp.objt-symbols.obj64 usr/src/uts/sparc/keysock/Makefile usr/src/uts/sparc/keysock/keysock.objt-symbols.obj64 usr/src/uts/sparc/neti/Makefile usr/src/uts/sparc/neti/neti.objt-symbols.obj64 usr/src/uts/sparc/os/device_policy usr/src/uts/sparc/rts/Makefile usr/src/uts/sparc/rts/rts.objt-symbols.obj64 usr/src/uts/sparc/spdsock/Makefile usr/src/uts/sparc/spdsock/spdsock.objt-symbols.obj64 usr/src/uts/common/inet/nd.c usr/src/uts/common/inet/snmpcom.c usr/src/uts/common/inet/tcp.h usr/src/uts/common/inet/tcp/tcp.c usr/src/uts/common/inet/tcp/tcp_fusion.c usr/src/uts/common/inet/tcp/tcp_kssl.c usr/src/uts/common/inet/tcp/tcp_opt_data.c usr/src/uts/common/inet/tcp_impl.h usr/src/uts/common/inet/tcp_stack.h usr/src/uts/common/inet/udp/udp.c usr/src/uts/common/inet/udp/udp_opt_data.c usr/src/uts/common/inet/udp_impl.h usr/src/uts/common/netinet/igmp_var.h usr/src/uts/common/os/netstack.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-2 Passed this file over to spend time on files which I could provide more help. usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-3 Line 634 T5 NIT -> free() works fine on NULL pointers. This check is unnecessary. DM-4 General T5/E5 Don''t forget to run cstyle -> I see some nits here like on line 1857. DM-5 find_all_inter- T3/E1 You have two big if branches. Why not split faces() it out into find_all_global() and find_all_one_zone(zoneid)? The codepaths are so different it makes NO SENSE to keep them crammed together in find_all_interfaces(). usr/src/cmd/cmd-inet/usr.sbin/wificonfig/wificonfig.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-6 Lines 4726-9 T4/E1 Why the second try? Shouldn''t you just use SYS_IP_CONFIG from the start? You need to explain a bit more here about what''s going on. usr/src/cmd/ipf/Makefile ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-7 Lines 2-21 E2 Are you supposed to CDDL this? Ask Darren Reed! usr/src/cmd/zoneadm/Makefile ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-8 Lines 70-72 T5 Isn''t this redundant? (See lines 58-59...) usr/src/cmd/zoneadm/dlprims.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-9 General T3 Don''t we have a libdlpi now for just this sort of occasion? usr/src/uts/Makefile ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-10 General E5 I like what you''ve done here w.r.t. checking. usr/src/uts/common/inet/arp/arp.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-11 Line 1093-97, T4 Is disallowing command from rput a distinct etc. bugfix? At least file one if it is, please. 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. usr/src/uts/common/inet/arp/arpddi.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-13 arp_ddi_*() T5 What problem did you solve by moving them into arp.c? usr/src/uts/common/inet/arp_impl.h ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-14 Line 171 T4 Why "void *" and not "struct ar_s *"? usr/src/uts/common/inet/common.h ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-15 ipdropper_t T3 The "because of include file order" seems like a cop-out. Maybe later I''ll see why, but it''s not clear to me why ipdropper_t had to move here. 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. 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? 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 usr/src/uts/common/inet/ip/ipsec_loader.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-19 Line 50 T3 Lose the ARGSUSED. usr/src/uts/common/inet/sadb.h ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-20 Line 236 T3 For an SA, which can be tied to a specific protocol, why are you pointing to the netstack_t instead of the AH or ESP-specific netstack pointer? Every call in ipsecah.c and ipsecesp.c seems to chase two pointers when you could chase one instead. And as for the sadb.c ones, you should keep the ipsec_netstack in each of the AH/ESP ones so the pointer chasing in these paths stays only one-level. (These paths are less performance critical than the ones you do in AH and ESP.) It''s *probably* too late to fix this prior to putback, but IMHO a P4 bug should be filed to track this. The de-STREAMS-ing of IPsec would probaly be the optimal place to fix this. I''m still a little disappointed more thought about how much pointer chasing wasn''t given here. usr/src/uts/common/inet/ip/ipsecah.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-21 Lines 943-4 T5/E5 Are these splits really necessary? Lines 1138-39 (These are in the FULL webrev too, so they aren''t full vs. reduced artifacts.) DM-22 Line 2711 T3 Move "io" to the main bunch of locals. Any compiler worth it''s weight will optimize out the difference. usr/src/uts/common/inet/ip/ipsecesp.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-23 Line 1840 T3 See DM-22. usr/src/uts/common/inet/ip/nattymod.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-24 Line 578 E5 Maybe a comment referencing ip_drop_init()? (It''s a nit, really.) 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? DM-26 ip_drop_packet() T3 The more I''m seeing them, the more I''d like calls a macro like: #define DROPPER(ipss, dropper) \ ((ipss)->ipsec_ip_drop_types == NULL) \ ? NULL : &((ipss)->ip_drop_types->(dropper)) to save on lines. DM-27 Lind 6402 E3 s/Appears/Is/. usr/src/uts/common/inet/ip/spd.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-28 Line 140 T2 Huh?!? What''s /dev/kmem got to do with anything? DM-29 Line 557+ E3 I think you applied the suggestion in FIXME, using ipsec_stack_t everywhere. 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. 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! DM-32 Line 2864, T3 If you''re serious about the checks in DM-26 e.g. and MD-31, you''re forgetting some in spd.c Why is that? 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). DM-34 Line 5738 E1 Excuse me? It *is* called... ;) usr/src/uts/common/inet/ip/spdsock.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-35 Line 1914 T2 Why ARGSUSED when they are being used? usr/src/uts/common/inet/ip/tun.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-36 General Looks okay for now, but MAAAAN when Clearview IP Tunnelling comes along, it''s gonna get different. Please be ready for Seb and/or I to ask a bunch of questions. usr/src/uts/common/inet/ipsec_impl.h ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-37 Line 657 T4 Action hashes may not need to be instanced. usr/src/uts/common/os/kmem.c ------- --------------- ------- ----------------------------------------------- No. Location Sev. Comment ------- --------------- ------- ----------------------------------------------- DM-38 General T5 Dumb question: why initialize here?
Dan McDonald wrote:> Sorry for not including the opensolaris list on this the first time. The > good news is that I remembered less than 24 hours after I sent the note to > Erik and friends.Thanks for the careful review. Responses and some followup questions below.> ===================== (Cut up to and including here.) ====================> > Reviewer Name: Dan McDonald > > Document/Module Title: IP Instances > > Document/Module Author: Erik Nordmark, Yukun Zhang, Dong-Hai Han > > Document/Module Version/Date: 3 December 2006 > > Reviewer Preparation Time: 10+ hours > >> ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-0 Looks good, no comments needed.OK> ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-1 Do not feel qualified to inspect this file.We have other reviewers that have looked at those files.> ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-2 Passed this file over to spend time on files which I could provide > more help.OK> usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-3 Line 634 T5 NIT -> free() works fine on NULL pointers. > This check is unnecessary.I think we''ll keep the check since this isn''t performance critical, and it makes it a bit more explicit for the reader.> DM-4 General T5/E5 Don''t forget to run cstyle -> I see some nits > here like on line 1857.The code passes cstyle(1). But I can split the assignment and if statement apart since that might reduce the number of line wraps. With DM-5 will have less indentation to make this easier.> DM-5 find_all_inter- T3/E1 You have two big if branches. Why not split > faces() it out into find_all_global() and > find_all_one_zone(zoneid)? The codepaths are > so different it makes NO SENSE to keep them > crammed together in find_all_interfaces(). >Will do. Helps reduce the indentation.> usr/src/cmd/cmd-inet/usr.sbin/wificonfig/wificonfig.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-6 Lines 4726-9 T4/E1 Why the second try? Shouldn''t you just > use SYS_IP_CONFIG from the start? You need > to explain a bit more here about what''s going > on.Given that wificonfig has recently been replaced by dladm, we will not modify wificonfig in our putback. And dladm has a reasonable way to handle privileges that allows show-* from non-global zones, hence it doesn''t need to be changed.> usr/src/cmd/ipf/Makefile > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-7 Lines 2-21 E2 Are you supposed to CDDL this? Ask Darren > Reed!We are not changing IP Filter Copyrights etc. I don''t know exactly what Darren''s agreement is with Sun on this.> usr/src/cmd/zoneadm/Makefile > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-8 Lines 70-72 T5 Isn''t this redundant? (See lines 58-59...)Yes. Will fix.> usr/src/cmd/zoneadm/dlprims.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-9 General T3 Don''t we have a libdlpi now for just this > sort of occasion?libdlpi plans to integrate in build 57 i.e. after IP instances. They know they can go ahead and remove this file when they do.> usr/src/uts/Makefile > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-10 General E5 I like what you''ve done here w.r.t. checking.OK> usr/src/uts/common/inet/arp/arp.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > 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.> 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);> usr/src/uts/common/inet/arp/arpddi.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-13 arp_ddi_*() T5 What problem did you solve by moving them > into arp.c?Make it more uniform with the ipddi.c/ip.c split. With IP Instances the init functions need to do some more work.> usr/src/uts/common/inet/arp_impl.h > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-14 Line 171 T4 Why "void *" and not "struct ar_s *"?Good question. In onnv we have this: static void *ar_g_head; /* AR Instance Data List Head */ and we just moved it to ar_stack_t structure. The as_head (and same for is_head in icmp.c) are only used by mi_* (e.g., mi_open_comm) routines, and those use void *.> usr/src/uts/common/inet/common.h > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-15 ipdropper_t T3 The "because of include file order" seems > like a cop-out. Maybe later I''ll see why, > but it''s not clear to me why ipdropper_t had > to move here.The issue is that the various header files with foo_stack_t need to know the size of ipdropper_t. We''ll change those header files to explicitly include ipdrop.h and move the declaration there.> 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.> 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?> 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 2051The 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.> usr/src/uts/common/inet/ip/ipsec_loader.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-19 Line 50 T3 Lose the ARGSUSED.Will do.> usr/src/uts/common/inet/sadb.h > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-20 Line 236 T3 For an SA, which can be tied to a specific > protocol, why are you pointing to the > netstack_t instead of the AH or ESP-specific > netstack pointer? Every call in ipsecah.c > and ipsecesp.c seems to chase two pointers > when you could chase one instead. > > And as for the sadb.c ones, you should keep > the ipsec_netstack in each of the AH/ESP ones > so the pointer chasing in these paths stays > only one-level. (These paths are less > performance critical than the ones you do in > AH and ESP.)That would mean either putting three pointers in ipsa_t instead of just the netstack_t (a ipsecesp_stack_t and ipsecah_stack_t) which uses more space, *OR* define it as a union, which means we loose the benefit of the compiler doing strict type checking.> It''s *probably* too late to fix this prior to > putback, but IMHO a P4 bug should be filed to > track this. The de-STREAMS-ing of IPsec > would probaly be the optimal place to fix > this. I''m still a little disappointed more > thought about how much pointer chasing wasn''t > given here.I''ve filed CR 6499955 to track this past our integration.> usr/src/uts/common/inet/ip/ipsecah.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-21 Lines 943-4 T5/E5 Are these splits really necessary? > Lines 1138-39 (These are in the FULL webrev too, so they > aren''t full vs. reduced artifacts.)I''ll undo the first one (The line is exactly 80 characters.) The second one is needed. The full webrev has 1138 ah1dbg(ahstack, ("Couldn''t find auth alg #%d.\n", 1139 assoc->sadb_sa_auth));> DM-22 Line 2711 T3 Move "io" to the main bunch of locals. > Any compiler worth it''s weight will optimize > out the difference.I wasn''t concerned with the compiler efficiency, but about the brain cycles of the person that will read this code and have to understand whether ii and io might be used incorrectly. Thus having local scope of io would seem to me to limit the scope of that concern. But I''ll do as you suggest.> usr/src/uts/common/inet/ip/ipsecesp.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-23 Line 1840 T3 See DM-22.Ditto.> usr/src/uts/common/inet/ip/nattymod.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-24 Line 578 E5 Maybe a comment referencing ip_drop_init()? > (It''s a nit, really.)I''ll add something.> 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?> DM-26 ip_drop_packet() T3 The more I''m seeing them, the more I''d like > calls a macro like: > > #define DROPPER(ipss, dropper) \ > ((ipss)->ipsec_ip_drop_types == NULL) \ > ? NULL : &((ipss)->ip_drop_types->(dropper)) > > to save on lines.I''ll add that.> DM-27 Lind 6402 E3 s/Appears/Is/.Will fix.> usr/src/uts/common/inet/ip/spd.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-28 Line 140 T2 Huh?!? What''s /dev/kmem got to do with > anything?Seems to be a leftover comment from when there were some globals. Will remove.> DM-29 Line 557+ E3 I think you applied the suggestion in FIXME, > using ipsec_stack_t everywhere.I actually haven''t. A number of the ipsec functions take a netstack_t pointer as the argument, and then use it to find the ipsec_stack_t, or the ah/esp stack_t. I don''t think it is hard to fix this. Should I give it a try?> 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> 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.> DM-32 Line 2864, T3 If you''re serious about the checks in DM-26 > e.g. and MD-31, you''re forgetting some in spd.c > Why is that?Might be due to merge history. But with your DROPPER macro all of them will test for NULL.> 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.> DM-34 Line 5738 E1 Excuse me? It *is* called... ;)Just checking ;-) I''ll remove the comment.> usr/src/uts/common/inet/ip/spdsock.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-35 Line 1914 T2 Why ARGSUSED when they are being used?Fixed.> usr/src/uts/common/inet/ip/tun.c > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-36 General Looks okay for now, but MAAAAN when > Clearview IP Tunnelling comes along, it''s > gonna get different. Please be ready for Seb > and/or I to ask a bunch of questions.Understood.> usr/src/uts/common/inet/ipsec_impl.h > > ------- --------------- ------- ----------------------------------------------- > No. Location Sev. Comment > ------- --------------- ------- ----------------------------------------------- > > DM-37 Line 657 T4 Action hashes may not need to be instanced.See response to DM-33.> 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. Erik
Erik Nordmark wrote:> .... > >> usr/src/cmd/ipf/Makefile >> >> ------- --------------- ------- >> ----------------------------------------------- >> No. Location Sev. Comment >> ------- --------------- ------- >> ----------------------------------------------- >> >> DM-7 Lines 2-21 E2 Are you supposed to CDDL this? Ask Darren >> Reed! > > > We are not changing IP Filter Copyrights etc. I don''t know exactly what > Darren''s agreement is with Sun on this.Makefiles, etc, in ON are the IP of Sun and aren''t considered a derived work. Darren
Darren Reed wrote:> Erik Nordmark wrote: > >> .... >> >>> usr/src/cmd/ipf/Makefile >>> >>> ------- --------------- ------- >>> ----------------------------------------------- >>> No. Location Sev. Comment >>> ------- --------------- ------- >>> ----------------------------------------------- >>> >>> DM-7 Lines 2-21 E2 Are you supposed to CDDL this? Ask Darren >>> Reed! >> >> >> We are not changing IP Filter Copyrights etc. I don''t know exactly what >> Darren''s agreement is with Sun on this. > > > Makefiles, etc, in ON are the IP of Sun and aren''t considered a derived > work.Thanks. I''ll make sure it is fixed. Erik
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 6506488Thank 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
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