Seb,
Here's my final round of feedback; sorry for being late. In general I
found the code to be an excellent refactoring of a thorny area -- nice
work! (There are a few IPsec-related pieces that seemed overcomplicated,
but looking at Nevada's tun.c they look at least as confusing before, so
on the whole it's still an improvement even in those areas.)
Webrev:
http://cr.opensolaris.org/~seb/clearview-iptun-cr3/
usr/src/uts/common/inet/iptun/iptun.c:
* 47: s/iptun_task_cv/iptun_task_cb/
* 51, elsewhere: Calling these primitives iptun_hold() and
iptun_release() is very confusing since those are usually the
names used for reference counting, which is of course totally
orthogonal to locking. Suggest iptun_enter() and iptun_exit().
* 62: What in <sys/strsubr.h> does this file rely on? That header
contains symbols private to the STREAMS framework.
* 77: What in <sys/ip_impl.h> does this file rely on?
* 84-86: This will erroneously consider the tuple
<IPTUN_TYPE_UNKNOWN, AF_INET> a match; I presume that never
happens, but a more explicit check could catch bugs earlier.
* 88: Appears to be an older version of IPTUN_HASH_KEY().
* 90-94: Seems like something that belongs in <inet/ip.h>
* 110: Either this belongs in <netinet/in.h> or it shouild be
prefixed with IPTUN_.
* 129: Why is this IPTUN_MIN_IPV6_MTU and not IPTUN_MIN_IPV4_MTU?
* 142: Unclear why iptun_hash_lock is not `static'.
* 144: Likewise, unclear why iptun_tunnelcount is not `static'.
* 226: Why is it not necessary to dispatch a IPTUN_TASK_LINK_UPDATE
to update the link state?
* 251: Will this force users of iptun_m_unicst() to have special
code to deal with iptun? Is it inappropriate to map this to
iptun_setsrc()?
* 259: Why EINVAL rather than ENOTSUP?
* 313-316: Why is this flag needed? Seems like you could just
test whether iptun_hoplimit == IPTUN_DEFAULT_HOPLIMIT in
iptun_headergen().
* 341: Unclear why we have this short-circuit path for MTU, but
not for hoplimit or encaplimit -- but as a result it seems one
cannot set a fixed MTU if it happens to be the same value as the
current (dynamic) MTU.
* 344: Given that `maxmtu' is dynamic, this seems a problematic
property to set programmatically since a call may or may not
fail depending on dynamic conditions. Further, if the call is
made in while maxmtu is a large value, that value remains set
even when maxmtu shrinks, which seems asymmetric. (IP has many
of the same problems with its current MTU handling.)
* 349: No way to cause IPTUN_FIXED_MTU to be cleared -- though the
same limitation also exists today with IP interfaces.
* 374-381: Would've expected this to be done by GLDv3, not by each
individual driver (sigh).
* 430: Would've expected the default to be the value that's
initialized in iptun_create() -- i.e. iti_maxmtu.
* 452-453: Delete extra blank lines.
* 490: Might be more accurate to name this iptun_enter_by_linkid()
or somesuch. (To someone used to standard terminology, this
looks like a standard lookup function that acquires a reference,
when in fact it holds a lock.)
* 492: Needless initialization.
* 507, 510, 514: s/defered/deferred/
* 558: Is there any other reason (besides a bug) that the lookup
could fail?
* 611-612: Seems like this should be an ASSERT(0) -- and probably
moved to the first `switch' statement.
* 634: `itd' leaked on ddi_taskq_dispatch() failure.
* 761: Seems needless to call iptun_taskq_dispatch() here if
IPTUN_CONDEMNED is set.
* 768: Unclear why iptun_headergen() returns a value since it
cannot fail. Can also remove associated error handling at
317-322, 335, 1362, and 1523.
* 840: Function name seems too general. Moreover, it seems like
there's one or more missing IPsec utility functions, as this
code looks tedious and repetitious.
* 865, 876, 885: No need to clean up partial established policy
on failure? Maybe that's handled by ipsec_swap_policy() in
the caller? This code is really obscure.
* 894-895: No idea what this comment means (and this function
could certainly benefit from a clear comment here).
* 909: Is this really worth three exclamations?
* 910: Supposed to be "&" rather than "&&"? Still not sure I
understand this check though -- e.g., why is it OK for
ipsr_self_encap_req to be IPSEC_PREF_UNIQUE?
* 925: Is this call to dls_mgmt_get_linkinfo() safe given that
iptun_lock is held?
* 935: Cast is only necessary because ipsec_actvec_from_req()
has a broken function signature.
* 949-991: The locking here might be fine, but I can't make heads
or tails of it. Seems like a lot of IPsec machinery is directly
exposed to consumers.
* 1016: Could be `static'.
* 1032: From the perspective of the caller, it's unclear what
iptun_setparams() may have done if the call fails and multiple
flags were set in itk_flags, which seems to limit the usefulness
being able to set multiple params at once.
* 1043, 1057: Outer `if' statement seems useless.
* 1079-1092: Punting this to the hapless caller via EAGAIN
complicates the interface with an implementation artifact.
Seems like another mechanism is needed here. (IIRC, GLDv3
ioctls are not allowed to block, which means the mechanism
might need to be quite a bit different.)
* 1098: As an aside, I'm unclear what constitutes a "simple"
policy. Is this defined somewhere?
* 1105: Can we make it here with `err' set to anything but zero?
* 1127: Why is m_min_sdu set to zero rather than
iptun->iptun_typeinfo->iti_minmtu?
* 1133-1136: Seems more straightforward as:
if ((err = mac_register(mac, &iptun->iptun_mh)) == 0)
iptun->iptun_flags |= IPTUN_MAC_REGISTERED;
mac_free(mac);
* 1192: Would seem more to the point to add a `default' clause to
the `switch' staement.
* 1197: This ASSERT() needs to be moved to 1193: we've
cleared CONN_INCIPIENT and dropped conn_lock, so conn_ref can be
increased by another thread.
* 1243-1244: Nothing seems to rely on iptun_free(NULL) working.
* 1245: Would be nice to ASSERT() IPTUN_CONDEMNED is set.
* 1265: Would seem reasonable to clear IPTUN_HASH_INSERTED here.
* 1339: What protects multiple simultaneous opens from both
finding iptuns_g_q clear and both proceeding to set it?
(Seems like iptuns_lock should protect this field.)
* 1340: Why is the default STREAMS queue instance associated with
the credentials of the iptun_create() call? Would've expected
zone_kcred().
* 1448: EACCES usually pertains to file permissions; would've
expected EPERM (and that's done at e.g. 1503).
* 1467, 1481: Still need to hold iptun_lock when clearing
IPTUN_CONDEMNED to guarantee the flag change will be visible to
other threads (usually done as part of the memory barrier as
part of mutex_exit()).
* 1482: What happens if dls_devnet_create() fails? Is it
appropriate to panic here?
* 1568: Extra blank line?
* 1572-1611: Seems like a general-purpose iptun_getaddr() function
or the like would be handy here.
* 1584, 1604: Isn't the caller (e.g., userland) responsible for
setting these flags? Seems like these would be EINVAL, not an
ASSERT().
* 1619: OK to access itp_flags without itp_lock held?
* 1620: I'm confused by the name/semantics of this flag -- the
code seems to use it to indicate that there is IPsec policy,
regardless of whether ipsecconf needs to be used -- e.g.,
ifconfig only advises using "ipsecconf -ln" when ITK_IPSECCONF
is set and ITK_SECINFO is clear.
* 1632, 1639: Why do these functions allow an error to be returned?
* 1658: If IPsec policy is removed entirely, what codepath leads to
iptun_update_mtu() being called?
* 1684, 1689: MATCH_IRE_DEFAULT means we won't necessarily return
the path MTU. Probably fine, but the result is not the path MTU.
* 1701: The function name traverse_rules() seems to have little to
do with what this function does (find the highest overhead).
* 1714: Something like iptun_get_ipsec_overhead() would be more
consistent with other names.
* 1736-1750: Seems like a utility function (in IPsec-land) that
initializes an ipsec_selector_t from a provided IPv4/IPv6
address and protocol would be useful.
* 1744: Takes as many lines of code to just do the obvious
assignment of B_FALSE ;-)
* 1768: "For now" meaning there's something else we should ideally
be doing? Perhaps the comment could be expanded?
* 1775: Could just pass 0 here rather than ipsec_ovhd.
* 1804, 1817: FWIW, there is a large project underway to remove
direct use of lbolt; would advise using ddi_get_lbolt() instead.
(Likewise with the usage in iptun_impl.h.)
* 1875: The comment is obvious from the code -- would be more
useful if the comment said why.
* 1902, 1942: Should a counter be bumped?
* 1899-1907, 1939-1947: Could have a utility routine for this.
* 2030: Worth ASSERT()ing that both ipha and ip6h aren't non-NULL
at the same time?
* 2069: Would be worth explaining why we don't need to worry about
packets that may not have an IP header's worth of data in their
first mp.
* 2095: Why MBLKL() again rather than using first_mblkl?
* 2097: What ensures this is safe -- e.g., what prevents
mp->b_cont->b_rptr from being a zero-byte message? Moreover, it
seems dangerous to provide the caller with a pointer to a header
that may be incomplete.
* 2141: Ultra-nit: s/.)/)./
* 2151: ASSERT(MBLKL(data_mp >= 0)) would be useful. Also, an
explanation of why this is safe.
* 2154: So something has already sanity-checked the inner packet
to ensure it's not garbage we'll misinterpret? Would be good to
explain those guarantees here.
* 2273: ASSERT(endptr < mp->b_wptr) would be useful, as would a
comment on why this is safe.
* 2276: Something guarantees that the resulting encaplimit option
entirely within the bounds of the packet, right?
* 2300-2410: Same comments apply as iptun_input_icmp_v4().
* 2429: Per PSARC/1997/198, this should be norcvbuf, not ierrors.
* 2439, 2444: Comment explaning why these casts are safe would be
useful.
* 2507: Block comment explaining the various types of M_CTL/M_DATA
conventions expected here would be useful -- e.g., M_CTL with an
M_CTL b_cont is a protected ICMP packet, but M_CTL with an
M_DATA b_cont is just a protected IP packet? Or something else?
The reader shouldn't need to have a deep knowledge of these
obscure conventions to understand this code.
* 2546: Why are these casts needed?
* 2665-2667: AKA `return (outer4->ipha_src != outer4->ipha_dst);'
* 2679: How does this differ from iptun->iptun_typeinfo->iti_minmtu?
* 2788: Would be good to ASSERT() limit is inside mp (looks to be
valid based on the current implementation).
* 2778: s/exceede/exceeded/
* 2841: Per PSARC/1997/198, this should be noxmtbuf, not oerrors.
* 2863: Some of the reasons for mp == NULL should bump noxmtbuf,
not oerrors.
* 2870: OK to access itp_flags without itp_lock held?
* 2871: Hoist ASSERT() to e.g. 2866? If this is not true, how is
the msgdsize() at 2895 safe?
* 2881: What cases cause us to get here with b_next != NULL, and
why do those conditions only apply when IPsec policy is active?
Would be nice to add a comment explaining this.
* 2891: Where is this done?
* 2921-2934: Would be more compact and clear to use C99
initializers -- e.g.:
static mac_callbacks_t iptun_m_callbacks = {
.mc_callbacks = (MC_SETPROP | MC_GETPROP),
.mc_getstat = iptun_m_getstat,
.mc_start = iptun_m_start,
.mc_stop = iptun_m_stop,
.mc_setpromisc = iptun_m_setpromisc,
.mc_multicst = iptun_m_multicst,
.mc_unicst = iptun_m_unicst,
.mc_tx = iptun_m_tx,
.mc_setprop = iptun_m_setprop,
.mc_getprop = iptun_m_getprop
};
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
