Hi Eric, Thanks very much for your review. Responses in-line:
On Tue, 2009-08-18 at 04:56 -0700, Eric Cheng wrote: > Hi Seb, > > I reviewed the mac related changes and glanced over iptun.c. I only have > a few comments: > > http://cr.opensolaris.org/~seb/clearview-iptun-cr2/ > > aggr_grp.c: > 1428: > you could change aggr_grp_delete() to pass in a cred_t so you don't have > to use CRED(). same issue with vnic_dev.c:589. ACCEPT > dld_drv.c: > 1272: > is this missing something? ACCEPT: This comment can be removed. A previous incantation of the code had the addition of a flags argument to dld_ioc_register(). > dld_proto.c: > 843: > why is the initialization needed? ACCEPT: It's not, this is a remnant of some past code churn here. Will revert. > > 868: > why is there no (void) cast in front of this call? (since this returns > a boolean_t). ACCEPT: This needs to be fixed, and lint isn't complaining. (!) This isn't the first instance of lint problems being found that seem to be masked by a buggy lint program... > dls_mgmt.c: > 786: > is this because the zone holds an extra reference so dls_devnet_rename() > can't reach here? Yes, dls_devnet_rename() returns with EBUSY is dd_ref > 1. > 813: > shouldn't the last arg be (mod_hash_val_t *)&ddp? ACCEPT > > 886: > the ddp could've been freed at 877 so this could access freed memory. ACCEPT: I need to only do this if err == 0. > 1148: > this assert doesn't seem valid all the time. (e.g. if the daemon dies > and couldn't handle the upcall) What upcall are you referring to? dls_devnet_hold() does not fail if dlmgmtd is unable to handle upcalls. It can only fail if the linkid doesn't exist in i_dls_devnet_id_hash, or if the dls_devnet_t is condemned. Because i_dls_devnet_create_iptun() succeeded, the entry must be there and isn't condemned. > > 1105: > I don't quite understand why you need to clear this flag. > i_dls_devnet_destroy_iptun() will call dls_devnet_destroy()-> > dls_devnet_unset() which does not re-enter dls_devnet_rele() > so iptun_delete() won't be called again. the comments also refers > to a non-existent function dls_devnet_unset_common(). ACCEPT: This is basically a mismerge with Crossbow. This code was necessary before Crossbow, where there was always an implicit reference. Now that Crossbow has removed the implicit reference, we don't need to worry about re-entering dls_devnet_rele() as a result of dls_devnet_destroy() indeed. Will remove the clearing of the flag here and the comment. > > 1758: > why doesn't this use CRED() like iptun_create()? Because the process to release the last reference to an implicit IP tunnel may not have the privileges to destroy IP tunnels. An implicit tunnel is created as a result of an open() in /dev/net. The process that did the initial open() must have the privileges necessary to create the tunnel, thus the CRED() call in iptun_create() at 1741. While the tunnel exists, a process with different privileges can open /dev/net/<tun> for other reasons (snoop for example). When the last process to have the device open closes it, the tunnel must be automatically deleted, which is why we use zone_kcred() and not CRED() for iptun_delete(). I'll add a comment. > mac_ipv4.c: > 195,196: > do you need to sanity check these lengths? ACCEPT: will add mblk boundary checks here. > iptun.c: > 214: > why not return an error if iptun_task_dispatch() fails? I don't think that failing to send an asynchronous link state notification to MAC clients is reason to fail the entire operation. Do you disagree? > 1046: > why is IPTUN_SRC/DST always cleared? if iptun already has a valid addr, > shouldn't that be kept even after an attempt to set an invalid one? Because we had to unbind with ip in iptun_modify(), and handling a failure case by issuing operations that can themselves fail (i.e., trying to re-bind to the old addresses) is not a good idea. If the administrator was trying to change the address, I think that reverting the tunnel to an unconfigured state if the operation failed is acceptable. The behavior is quite deterministic and makes the failure obvious. > > 1488: > the done label seems unused. ACCEPT > > 2017: > add check for MBLKL(mp) > (ipha_t + icmp_t) or (ip6_t + icmp6_t)? ACCEPT: I won't do that here, but in the common iptun_input_icmp() input path for all ICMP packets. > > 2059: > add check for first_mblkl is > ipha_t? ACCEPT > > 2424: > possibly more header size checks needed. ACCERPT: this is where I'll add the checks you suggested for 1017. > > 2571: > may be better to break up the chain to get the actual drop count. ACCEPT Thanks, -Seb
