Girish M G writes:
> The webrev is located here:
> http://cr.opensolaris.org/~girishmg/tcpsig/
ipsecconf.c
839-843: shouldn't the known algorithm list returned by the kernel
include the new TCP-MD5 feature?
1999: suggest parenthesis around '&' expression used as boolean.
3531: how does this differ from just "strcasecmp"?
ipseckey.c
606-607: how does this differ from strcasecmp?
932: what does this do?
933: this is hard to read ... would isascii() and isgraph() help?
1013: shouldn't flush work?
spd.c
3203: xxx comment; please resolve.
ip_stack.h
418: why void *?
tcp.h
613,614: could these be something other than void *?
tcp.c
650: suggest uint_t here.
5822,13449,13783: use != instead of ^.
5827-5828,13456-13457,13790-13791: cstyle: missing blank line
between definition and executable code.
5831,13450,13461,13784,13795: this doesn't look like a good choice
to me. A remote attacker could spam the system log. I suggest
either a dtrace probe point or just nothing at all.
12764: this shouldn't be necessary. The arguments are used.
12773: this isn't necessary; every dereference comes with a free
assertion that the pointer isn't NULL. Plus, the caller uses "&".
13462,13796: how is printing the pointers helpful?
13787,13800: we shouldn't be going through the ack logic *after*
having failed signature validation, should we? Failing to validate
means that the packet is from an attacker.
16222-16236: repeated code; looks like something that should be a
generic ipsec function.
16231: either "1" or B_TRUE would be more legible here.
18277: use tcp_signature as boolean here (remove "!= 0"), or use as
numeric throughout.
20255-20256: could be a little more upbeat here: packets needing TCP
signature aren't sent down the MDT path because they're not
typically the continuous bulk data sort of traffic for which MDT was
designed, and because the complexity of updating MDT to sign each
packet outweighs the marginal performance gain.
22021: comment needs to be updated; we branch off if a signature is
needed or if ...
23706: it looks like there are other definitions of wptr in this
function that shadow this one.
23719,23729: combine with "&&".
23723-23726: nit: could keep same margins here. (And if you're
going to change this comment, sprucing up the grammar would be
nice.)
24062,24075: these hard-coded constants probably belong somewhere
else ... aren't they just TCPOPT_SIG_LEN + 2?
24227: ... or where a TCP signature is needed.
24241,24299: what is this change for? It seems unrelated.
pfkeyv2.h
589: I think this should match BSD.
782: random blank line?
tcp.h
92,94: these aren't needed. The other things in this file have
#ifndef/#endif brackets because of XTI. See the comment on lines
75-78.
93: is this actually used?
tcp_sig.c
29-66: that's some list of #includes. Are all of them needed?
Including the ndd bits? I suggest running lint with
'LINTFLAGS=-axsNlevel=2' to see if maybe some of these are
unnecessary.
70: I suggest using sys/list.h rather than rolling your own linked
list implementation.
74: design is unclear; this lock doesn't seem to protect much. The
assignments to and tests of tsa_deleted are all single-instruction
statements, so the lock there (lines 357-359,566-583) does nothing.
The only thing actually serialized is the block from 665-710, but
that one doesn't make sense -- why prevent multiple simultaneous
_readers_ of this data, and not block out _writers_? (In fact, the
contents seem to be protected by tdb_lock -- but the reader doesn't
take this lock.)
108: duplicate #define; use MBLKL instead.
109-116: please remove and just use htonl/ntohl.
147: s/Recieved/Received/ (But suggest dtrace instead.)
171: for safety, I think you may need to write to a local in_addr_t,
and then assign that to sin->sin_addr. There's no guarantee that
this can be done in place.
189,190,298,376: initialized values not used.
214-217: what are these?
226-227,321-322,380-381: these don't seem much used; could they be
inlined at 229,233,324,328,383,387? Or perhaps a switch used as in
line 472?
234,238,329,333,388,391: please remove debug printfs.
235,330: ADB_X_DIAGNOSTIC_BAD_SRC_AF, perhaps?
243: doesn't seem to test much.
246,339,397: explicitly compare tsa against NULL.
254,266: may want to do as a single allocation request.
262-263: does the length-limited copy help? Why not just do
structure assignment?
357-359: assignment at 358 is a single instruction; locks don't do
anything.
383-393: once the debug printfs are gone, this reduces to just:
if (src->ss_family != AF_INET)
return (NULL);
len = sizeof (struct sockaddr_in);
tdb = ipst->tcp_sig_db[TCP_SIG_OUTHASH4(
((struct sockaddr_in *)dst)->sin_addr.s_addr)];
398-416: having a per-AF socket comparison function that understands
port==0 as a wildcard would be cleaner than wiping out the port
numbers and then restoring them later just so that you can use bcmp.
409-410: I don't think this works. Dropping tdb_lock and then
returning means that the 'tsa' pointer returned is potentially
invalid by the time the caller gets it. Instead, this function
should do TSA_REFHOLD *before* dropping tdb_lock, and always return
with a reference held. The caller needs a reference anyway.
430: why does this need to be TCP_SIG_BUCKETS separate allocations?
473: zero?
474,502: extra block structure (curly braces) not necessary; no
local definitions.
477-489: this code seems misplaced. It's actually needed only when
we search for an SA for the first time -- at line 555. We shouldn't
need to do this work on every packet.
482,489: bcopy?
504: probably not useful; we can't get here.
519: could this happen? If so, is asserting the right thing to do?
521-522: this doesn't seem to be necessary. The code that follows
will handle len <= 0.
526,533: not needed; assert comes for free with dereference.
541,553-584: this can be greatly simplified. The done_payload label
isn't needed and most of the "inbound" tests are unnecessary. Just
do this:
if (tsa != NULL && tsa->tsa_deleted) {
if (inbound)
tcp->in_tcp_sig_sa = NULL;
else
tcp->out_tcp_sig_sa = NULL;
TSA_REFRELE(tsa);
tsa = NULL;
}
if (tsa == NULL) {
/* fill in src/dst here */
tsa = tcpsig_find_sa(mp, &src, &dst, ipst);
if (tsa == NULL)
return (B_FALSE);
/* note that tcpsig_find_sa returns with ref held */
if (inbound)
tcp->in_tcp_sig_sa = tsa;
else
tcp->out_tcp_sig_sa = tsa;
}
545:nit: s/TCP Signature normally do not/TCP Signature key normally
does not/
613: stray blank line in middle of definitions.
665: this lock doesn't protect anything that this function is
referencing.
712: what is this about?
--
James Carlson, Solaris Networking <[EMAIL PROTECTED]>
Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084
MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
_______________________________________________
networking-discuss mailing list
[email protected]
