Thanks Jim for the detailed code-review. Please see in-line for my
responses.
The webrev (against the onnv-gate) is at
http://cr.opensolaris.org/~girishmg/tcpsig_latest/
and the webrev (just your changes against my workspace) is at
http://cr.opensolaris.org/~girishmg/tcpsig_jim_changes/
James Carlson wrote:
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?
That would mean that the IPsec infrastructure supports MD5 for AH and
ESP also. However the only MD5 type algorithm supported by AH and ESP is
MD5-HMAC. So I had to work around this issue.
1999: suggest parenthesis around '&' expression used as boolean.
Done
3531: how does this differ from just "strcasecmp"?
It doesn't. So changed to strcasecmp.
ipseckey.c
606-607: how does this differ from strcasecmp?
It doesn't. So changed to strcasecmp.
932: what does this do?
933: this is hard to read ... would isascii() and isgraph() help?
True isgraph() would make it easier. So used isgraph().
1013: shouldn't flush work?
No not right now. It will be tracked as an RFE. The user need to use
DELETE to delete the SA's. The Flush and Dump SADB_MSG's have a
different codepath in the current gate and to make those things work for
TCP Sig means duplicating lot of IPsec infrastructure. Further when
these modules keysock, ah & esp are merged it can be easily implemented.
spd.c
3203: xxx comment; please resolve.
Resolved.
ip_stack.h
418: why void *?
The hash table structure which holds the TCP signature SA's are defined
inside tcp_sig.c and are opaque. So defined it to be void *.
tcp.h
613,614: could these be something other than void *?
The TCP signature SA structure are defined inside tcp_sig.c and are not
visible outside that file and hence it's 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.
Done.
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.
Am using the Dtrace probes at all the locations. Please let me know if
it's ok.
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 "&".
Ok.
13462,13796: how is printing the pointers helpful?
Removed. Now I have Dtrace probes.
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.
Yeah, we don't need the ack logic. So I just return from tcp_rput_data()
after freeing mblk.
16222-16236: repeated code; looks like something that should be a
generic ipsec function.
Created new function ipsec_tcp_signature_set() as part of spd.c.
16231: either "1" or B_TRUE would be more legible here.
Used B_TRUE.
18277: use tcp_signature as boolean here (remove "!= 0"), or use as
numeric throughout.
Now am using it as a boolean 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.
Update with above comment.
22021: comment needs to be updated; we branch off if a signature is
needed or if ...
Added.
23706: it looks like there are other definitions of wptr in this
function that shadow this one.
There is only one *wptr now throughout the function.
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.)
corrected both above
24062,24075: these hard-coded constants probably belong somewhere
else ... aren't they just TCPOPT_SIG_LEN + 2?
Yes. But we can't directly add it as the data offset field of tcp header
is 4 bits
and since these 4 bits are from MSB, we need to use the left shift
operator. I
have updated the comment.
24227: ... or where a TCP signature is needed.
Added to the comment.
24241,24299: what is this change for? It seems unrelated.
True. Changed it.
pfkeyv2.h
589: I think this should match BSD.
Have noted this down and will talk with Dan and you, get this fixed.
782: random blank line?
Removed the 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?
No. Removed it.
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.
Ok did that.
70: I suggest using sys/list.h rather than rolling your own linked
list implementation.
Have used it. Please check the changes for correct initialization and usage.
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.)
True got rid of the lock which was neither really protecting anything
nor was it serializing any stuff.
108: duplicate #define; use MBLKL instead.
109-116: please remove and just use htonl/ntohl.
Done
147: s/Recieved/Received/ (But suggest dtrace instead.)
Used Dtrace probe.
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.
Corrected all above.
214-217: what are these?
Questions to IPsec code-reviewers. In the passed down SADB message
should I ensure whether certain fields are what it is supposed to be.
Actually none of them is required. So have removed them.
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?
Corrected all above.
243: doesn't seem to test much.
True. Removed
246,339,397: explicitly compare tsa against NULL.
Done.
254,266: may want to do as a single allocation request.
Would not know the complete size until later. So retained as is.
262-263: does the length-limited copy help? Why not just do
structure assignment?
Did structure assignment.
357-359: assignment at 358 is a single instruction; locks don't do
anything.
Removed.
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)];
done.
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.
Have done that. Please look at tcpsig_compare_addr() function in tcp_sig.c
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.
True. I used to hold the reference in the calling function. Now before I
drop
the lock I call TSA_REFHOLD and then return.
430: why does this need to be TCP_SIG_BUCKETS separate allocations?
with those many hash buckets the distribution of the keys would be well
spread. It's set to 256 for now. Are you suggesting a single bucket with
keys in them? or some smaller values of buckets.
473: zero?
474,502: extra block structure (curly braces) not necessary; no
local definitions.
Removed
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.
We also need it for the computation of MD5 for the packet (mblk_t) passed
to tcp_signature().
MD5Update(&ctx, (char *)&ip->ipha_src, sizeof (ip->ipha_src));
MD5Update(&ctx, (char *)&ip->ipha_dst, sizeof (ip->ipha_dst));
482,489: bcopy?
yes th->th_lport and th->th_fport is an uint8_t array of size 2 and I
need to
copy the value to in_port_t and hence bcopy.
504: probably not useful; we can't get here.
true. removed
519: could this happen? If so, is asserting the right thing to do?
No. Removed the assert.
521-522: this doesn't seem to be necessary. The code that follows
will handle len <= 0.
True. Removed it
526,533: not needed; assert comes for free with dereference.
Removed
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;
}
True. Have modified as suggested
545:nit: s/TCP Signature normally do not/TCP Signature key normally
does not/
613: stray blank line in middle of definitions.
Ok.
712: what is this about?
when "ipseckey get tcpsig" is called from CLI,
tcpsig_delget_sa(delete=B_FALSE) is
called. This will hold a reference on the "tsa". Now we need to convert
this "tsa" into
a new SADB message and reply back to the consumer. This is done by
tcpsig_newsamsg(). Once am done with building the message I release the
hold on *tsa*.
665: this lock doesn't protect anything that this function is
referencing.
True. The initial intent was to disallow update to a TCP signature SA (tsa)
while retrieving that SA from ipseckey. However we don't allow any updates
on an existing *tsa* as it will be in Mature state. So this lock doesn't
protect
anything. So removed it.
_______________________________________________
networking-discuss mailing list
[email protected]
