On Wed, Jul 29, 2009 at 04:14:22PM -0400, Sebastien Roy wrote:
> This is a invitation to all to participate in the Clearview IP Tunneling
> code-review. Please send any comments to
> clearview-discuss at opensolaris.org before August 12th.
Cutting out networking-discuss, then. Please include me (danmcd) in replies
on this sub-thread, because I'm not on clearview-discuss. I'll try and make
sure to read the Jive Forum version, too.
> Special thanks go out to Dan McDonald who volunteered his time to
> integrate IPsec support into the new iptun driver.
You're welcome. You've cleaned up a lot here, and it was the least I can do.
Anyway, on to the bits you pointed me at.
Dan
===================== (Cut up to and including here.) =====================
Reviewer Name: Dan McDonald
Document/Module Title: Clearview IP Tunneling - IPsec set + iptun.c general
(iptun.c, sadb.c, spd.c, spdsock.c, plus others)
Document/Module Author: Sebastien Roy
Document/Module Version/Date: 1 August 2009
Reviewer Preparation Time: 3-4 hours.
usr/src/uts/common/inet/ipsec_impl.h
usr/src/uts/common/inet/iptun.h
usr/src/uts/common/inet/ip/sadb.c
usr/src/uts/common/inet/ip/spdsock.c
------- --------------- ------- -----------------------------------------------
No. Location Sev. Comment
------- --------------- ------- -----------------------------------------------
DM-0 Looks good, no comments needed.
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c
------- --------------- ------- -----------------------------------------------
No. Location Sev. Comment
------- --------------- ------- -----------------------------------------------
DM-1 set_tun_algs() T2 Regression w.r.t. tun_reality_check. We're
going to have to figure out something here.
NOTE: We had an internal mail thread, and the
correct answer is to re-introduce the
tun_reality_check functionality in a
different way across the invocation of
ifconfig(1M). I will post an
edited-for-brevity transcript of this mail
thread on a seperate note to
clearview-discuss
usr/src/uts/common/inet/iptun/iptun.c
------- --------------- ------- -----------------------------------------------
No. Location Sev. Comment
------- --------------- ------- -----------------------------------------------
DM-2 Line 37 E4 XXX comment should be filled in with locking
notes - especially that "hold" means "lock
with checking" in iptun.
DM-3 Early on, until T3/E3 Several things here seem like they should be
line 92-ish. defined in iptun.h or iptun_impl.h. Is
there a reason they aren't?
DM-4 Line 223 T1 Why would iptun_m_multicst() fail?!?
Multicast over a point-to-point link is a
no-brainer, not unlike promiscuous (which you
have succeed). Plus, you can replace those
godawful hacky multicast routing ioctls with
better configured iptun interfaces this way.
(I wish I'd better grasped that concept back
in the day -- Deering's mods were pretty
hacky.) Consider the punchin client that
wants to multicast over the SWAN, for
example.
Or is this a link-layer concept that I'm
blowing steam over?
DM-5 Lines 306-9 T5 Nit --> why EINVAL for both cases?
One error is IPTUN_TYPE_IPV4, which perhaps
could be EPROTONOSUPPORT? And the other is a
too-large value... perhaps E2BIG? Or is an
app expecting EINVAL for all of these?
DM-6 Lines 435-6 E3 Comments don't match code. I see no
reference to "iptuns", e.g.
DM-7 iptun_unbind() T4/E2 You are tweaking a flag, but therre's no sign
of locks being held. Can you at least place
an ASSERT(), a comment, or something to
indicate it's safe to modify iptun_flags?
DM-8 Line 772 T5 If someone uses ndd and sets
ip_path_mtu_discovery to off, should you be
setting IPH_DF?
DM-9 Line 1133 T4 Someone should be giving you an answer to
that question.
DM-10 Lines 1143-56 E3/T4 So let me get this straight. conn_zoneid is
set to GLOBAL if the tunnel is for an
exclusive-stack zone OR the tunnel is in the
actual global zone (per crgetzoneid())?
How will this play (or not) with TX? Have
you had Jarrett or Ken look at this?
Also, iptun_zoneid is just set to the zone
ID of whatever plumbs the tunnel (regardless
of shared or not)?
Finally, we've talked about split-zone
tunnels (lower-layer/conn_t in one zone,
upper-layer/GLDv3-stuff in another) -- will
there need to be extra magic here if we pull
this off?
DM-11 Line 1378 T3 Looking from the contents of modhash.c, why
are you locking iptun_hash at all? It
appears that modhash.c's implementation
enforces a reader/writer lock around this
already. Is it because of the corner-case in
iptun_task_cb()? You also mention a condvar
up toward the top. That too?
If either of those are the case, make sure
you mention that at the top of iptun.c.
usr/src/uts/common/inet/ip/spd.c
------- --------------- ------- -----------------------------------------------
No. Location Sev. Comment
------- --------------- ------- -----------------------------------------------
DM-12 itp_get_byaddr() T5 NICE --> I forgot how you cleaned up the
whole function-pointer mess since one can
just use ipclassifier.c functions now.
