On 13/08/09 08:44 PM, Eric Cheng wrote:
On Mon, Jul 27, 2009 at 07:01:58PM -0700, Darren Reed wrote:
http://cr.opensolaris.org/~darrenr/onnv-pcapture/
Hi darren,
Here are my comments for the mac and sockmod_pfp components:
dls_link.c:
930:
dls_link_getzid() seems to be unused.
Discuss.
In testing, zone_access_datalink() didn't work as expected, thus it has
been thrown to /dev/null and dls_link_getzid() used instead (code path
now works as expected.)
mac_client.c:
3253-3254:
I think you can't assume that the mp_copy is still valid after
invoking mpip->mpi_fn(). mp_copy could already be freed.
Discuss.
The basis of "mp_copy == mp" is the mpi_no_copy being true.
Users of the interface that do not request the mblk to be copied
(by using MAC_PROMISC_FLAGS_NO_COPY) are not entitled
to modify the mblk_t that is passed in. That includes prohibition
from free'ing them. Or is there some other code path that will
free the mblk_t whilst mpi_fn() is being called?
to fix this, you'll probably need to move the mpi_no_copy logic
to mac_promisc_dispatch() and mac_promisc_client_dispatch(). the
b_next should be saved before calling mac_promisc_dispatch_one().
also, if mpi_no_copy is set, the mp_chain has to be freed
right after mac_promisc_dispatch()/mac_promisc_client_dispatch()
because you can't let the same mp_chain come up two different paths.
bpf.c:
1710-1720:
if I read this correctly, this is passing the mac name to
mac_client_open() which will result in the mac name being copied
to bif_ifname. I think you want the real link name right?
Yes.
to get the real link name, the name arg to mac_client_open()
should be NULL and flags should include
MAC_OPEN_FLAGS_USE_DATALINK_NAME.
Accept.
also, in several places in this file you have something like:
if (bpf_debug)
cmn_err(CE_WARN, "...");
it may be better to put this into a macro.
Reject.
Putting debugging things in a macro is not friendly to having multiple
arguments to the "printf" function without ugliness like BPF_DEBUG1(),
BPF_DEBUG2(), etc. At least as far as I know, there are no stdargs
macro variations.
bpf_mac.c:
102:
do you need to return the correct size for non-ether macs?
the bpr_hdr_length entry point also seems to be unused.
Accept.
(It will be removed.)
sockmod_pfp.c:
1211:
you should consider adding a pfp_close() function symmetrical
to pfp_open_index() for closing mh/mch.
Reject.
There would only be one use of pfp_close(), whilst there are multiple
places that need pfp_open_index()
1223:
similar problem as bpf.c.
you should pass a NULL name and MAC_OPEN_FLAGS_USE_DATALINK_NAME
flag to mac_client_open(), then use mac_client_name() to get
the link name and pass it to zone_access_datalink().
Accept.
716,719:
if you did the pfp_open_index() at 660, these conditions must
be true. you could set a boolean at 660 and use it instead
of checking mh/mch. something like:
if (new_open) {
ASSERT(mch != ps->ps_mch);
ASSERT(mh != ps->ps_mh);
pfp_close(mch, mh);
}
Discuss.
So you only wanted pfp_close() to do the mac_client_close and the
mac_close? That seems hardly worth it... but the new_open change
is worthwhile.
722:
mp could be uninitialized if you jumped from 674 and this would
free invalid memory.
Accept.
(I'm surprised neither lint nor the compiler found this, well done!)
330:
you need to specify MAC_PROMISC_FLAGS_VLAN_TAG_STRIP in
mac_promisc_add() otherwise the mhi_bindsap here will be
incorrect for vlan packets.
Discuss.
Does this comment apply to all uses of mac_promisc_add()?
I couldn't find any that are at line 330...I think you mean 1330?
356:
shouldn't you use msgdsize() instead of MBLKL?
Reject.
No. The code wants to know if the entire packet is in one mblk_t or not
so that it can optimise later functions - for example, if it is, then it is
safe to use bcopy() as the "copy function" for packet data and if it is
not, then you need to use a function that can walk mblk_t's.
408:
mhi_saddr could possibly be NULL (for IB links)
Accept.
976:
why doesn't this function use dls_mgmt_get_linkid()?
Accept.
955,982:
should this be copyin() instead of bcopy()?
Accept.
1150:
if possible, try to limit bf_len/bf_insns to some reasonable
number so you don't allocate too much kernel memory.
Reject.
This is enforced by bpf_validate.
1180:
need to return an error if bpf_validate() fails.
Accept.
1160:
maybe pfp_release_bpf() should be done after bpf_validate().
Accept.
And in addition, there needs to be a lock used to guard against
pfp_release_bpf() being called whilst bpf_filter() is executing
using that buffer.
Darren
_______________________________________________
networking-discuss mailing list
[email protected]
