Thanks for the comments, Please find responses below.
thanks,
-venu
>
>Here are my comments:
>
>usr/src/cmd/dlstat/dlstat.c
>usr/src/cmd/flowstat/flowstat.c
>
>SR1: * General: These work in non-global zones, correct?
>
Yes.
>
>usr/src/uts/common/io/dld/dld_drv.c
>
>SR2: * 436: You removed the zoneid checks here, why? This means that any
> zone can see every other zones'' datalinks'' ring
grouping
> information, which is a leakage of information across zone
> boundaries is it not?
ACCEPT. Good catch. Looks like a merge issue, will fix.
>
>
>usr/src/uts/common/io/mac/mac_sched.c
>
>SR3: * 464: Tracking down where this hint comes from is quite difficult.
> Some more comments about where this 64-bit value comes from would be
> helpful.
ACCEPT
> It looks like in most cases, it comes from
> CONN_TO_XMIT_HINT() or ILL_RING_TO_XMIT_HINT(), which both go out of
> their way to cast the pointers used as hints to 32-bit values. Why?
> Also, given the HASH_HINT() algorithm, I don''t see how those
macros''
> bit-shifting magic does anything useful.
Agree. CONN_TO_XMIT_HINT() or ILL_RING_TO_XMIT_HINT() need not do bit
shifting
>
> I have the same question for the similar code in aggr_send.c.
>
>SR4:* 482, 507, 534: The MAC name isn''t the same as the link name.
I
> suspect that things may be broken with renamed links.
>
Which file is this?
>
>usr/src/uts/common/io/mac/mac.c
>
>SR5: * 323: Comments are needed to explain what this taskq is used for.
>
ACCEPT
>SR6:* 2844: It looks like this function should be checking the size of all
> public properties (given how it''s used in dld_drv.c), but this
isn''t
> the complete set of public properties... For example, you removed
> the property size checks out of iptun.c, but did not replace that
> code with any checks for those properties anywhere else. What about
> WiFi properties etc...?
ACCEPT - Will add the missing checks
>
>SR7: * 3526: typo, remove "be".
>
ACCEPT
>SR8: * 3905: among
ACCEPT
>
>SR9: * 6294: close parens
>
ACCEPT
>SR10: * 6502: Need blank line between variable decl. and code.
ACCEPT
>
>SR11: * 7324: What prevents the mip from being destroyed right before you
> call i_mac_perim_enter() here? Comment please.
We are aware of this issue and currently looking at fixing this.
(CR 6812110 is also related to this issue). Will get back on this.
>
>
>usr/src/uts/common/io/mac/mac_stat.c
>
>SR12: * 312: Using kstat_create() means that this stat will be visible by
> all zones, which is likely not what is intended for statistics
> associated with objects that only exist in one particular zone. See
> kstat_create_zone(). There is also a pre-existing bug of the same
> nature at line 795.
>
I think one can call kstat_create_zone from the zone context when the zoneid
is known. However, in this case, link creation (mac registration and
thus the kstat creation) will typically happen before the link is assigned to
a zone. Thus, when zonecfg assigns a link to a zone, we will need to destroy
and recreate kstats or have an API like kstat_modify_zone.
It seems this may not have an easy fix and thus will suggest that I will file
and RFE to track this issue.
Meanwhile, I will like to note that these newly introduced stats do not reveal
any information that was not visible from within a zone before. (i.e. one could
do a kstat ixgbe:0 from within a zone and get access to driver stats even if
that interface is not assigned to the zone).
>
>usr/src/uts/common/io/mac/mac_protect.c
>
>SR13: * General: It may be a good idea to assert that appropriate mutexes
> are held in the various routines that directly access the avl trees
> (e.g. insert_dhcpv4_pending_txn() and similar functions) since they
> don''t have any locking themselves.
>
ACCEPT
>SR14: * 136: Remove extra empty comment line.
>
ACCEPT
>SR15: * 144: This is trivial to implement (increment a statistic when the
> limit is hit), why not do that now?
>
Accept. will add new stats.
>SR16 * 297: Can valid DHCP packets be fragmented? If so, then why would
> you reject them? If you reject such packets, then there could be a
> security hole as link protection will silently be inactive.
>
fragments are ignored because we don''t yet support fragment reassembly
in the mac layer. yes, this is a security hole for the dhcp-nospoof
case. to address this, we will drop all dhcp fragments if dhcp-nospoof
is on. for fragments that couldn''t be identified as dhcp (because
it''s too small to fit a udp header), they will be dropped as well
because they don''t conform to the minimum ip fragment size of 68
bytes (RFC 791). this check will be done on the initial fragment only.
for ipv6, there is a min mtu requirement of 1280 bytes. we will drop
initial fragments smaller than this size if dhcp-nospoof is on.
>SR17: * 748: In the IPv4 case (at line 297), you reject IP fragments but you
> don''t do anything equivalent here for IPv6. If there''s
a valid
> reason to reject fragments (which is the subject of my previous
> comment), then you should do so consistently.
>
Accept. will reject v6 fragments as well.
>
>usr/src/uts/common/io/mac/mac_util.c
>
>SR18: * 473: What are these diffs against? The onnv version of this
> function has two more arguments (ip_fragmented and ip_frag_ident),
> neither of which are in the left side of the diffs... Color me
> confused.
this was diffed against onnv_130, which didn''t have the new args
introduced recently.
>>>>> We''d like to make the code available for review
for the following
>>>>> additions as part of follow-up to the crossbow project:
>>>>>
>>>>> PSARC/2009/638 Public GLDv3 Interfaces
>>>>> PSARC/2009/501 Dynamic Ring Grouping on NICs
>>>>> PSARC/2009/448 pool dladm link property
>>>>> PSARC/2009/436 Anti-spoofing Link Protection
>>>>> PSARC/2009/364 dlstat and flowstat
>>>>>
>>>>> Webrev: http://cr.opensolaris.org/~tlc/crossbow_onnv130
>>>>>
>>>>> The changes also include some bug fixes. The changes/webrev
is based on
>>>>> build 130. We will update the webrev once in a couple of
weeks time to
>>>>> include some minor bug fixes, however we''d like to
get started on the
>>>>> code review now.
>>>>>
>>>>> Thanks in advance, to Sebastien, Cathy, Girish and Garrett
who have
>>>>> agreed to review the changes.
>>>>>
>>>>> Please send comments etc. to crossbow-discuss at
opensolaris.org by
>>>>> the 15th of Jan. If anyone needs more time, please let us
know.
>>>>>
>>>>> For internal folks, the webrev is @
>>>>>
/net/nvtbld-x.sfbay/exportz/crossbow/crossbow_onnv130/webrev
>>>>>
/net/nvtbld-x.sfbay/exportz/crossbow/crossbow_onnv130/webrev-closed
>>>>> The above also has cscope built.