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 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.
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
_______________________________________________
networking-discuss mailing list
[email protected]
