I'm assuming, Meem, that you're *on* clearview-discuss. I'm also Bcc:-ing
the internal IPsec list, in case I missed something.
> Post-Clearview-IPMP, one thing we need to be especially careful with is
> ill indices. Specifically, since data addresses are on the IPMP group
> interface (which has its own ill index) and packets are sent on one of the
> group's underlying interfaces (each of which has its own ill index),
> things that record or use ill indexes need to be precise about which one
> they mean to use. As such, I'm looking at the IPsec ill index usage.
> It seems like we have:
>
> * ipsec_in_ill_index: the ill index tied to the IRE_LOCAL for the
> given packet. Appears to be used primarily to restart inbound
> processing in ip_fanout_proto_again() (when IPsec prevented that
> from being done in ip_input()), and for setting
> ipsec_out_ill_index via ipsec_in_to_out(). Post Clearview-IPMP,
> ipsec_in_ill_index will be an IPMP group ill index for IPMP data
> addresses.
That makes sense.
> * ipsec_in_rill_index: the ill index tied to the interface that
> actually received the packet. Appears to only be used to
> restart inbound processing in ip_fanout_proto_again().
> Post-Clearview-IPMP, ipsec_in_rill_index will *always* be the
> ill index of the underlying interface that received the packet.
This also makes sense.
> * ipsec_out_capab_ill_index: the ill index ipsec_out_process()
> thinks the packet will be sent on, which will later be validated
> before actually transmitting with acceleration. Seems like this
> code may no longer be needed post-vca, but in any case,
> post-Clearview-IPMP, this will always be an underlying
> interface.
We're hoping to get rid of this really soon anyway. Are there plans for
Clearview-IPMP to target an S10 update? If not, your gate could make a nice
repository for the removal of this code. ;)
> * ipsec_out_ill_index: it *seems* like this should be the ill
> index tied to the source address for the packet. This is based
> on my observation of its primary[2] use post-Clearview-IPMP: it's
> how ip_wput_ipsec_out{,_v6}() get back the context of the ill
> they're working on (e.g., to eventually call ip_newroute_*()).
> That means e.g. ip_wput_ire() needs to set ipsec_out_ill_index
> to ire->ire_ipif->ipif_ill rather than stq->q_ptr. Will that
> cause problems?
It shouldn't.
> Related to the semantics for ipsec_out_ill_index: I see that
> ipsec_out_process() directly passes an ill into ip_wput_ipsec_out{,_v6}(),
> and that this ill is based on ire_stq rather that ire_ipif. So, if we go
> with the above definition for ipsec_out_ill_index, it'd seem that
> ipsec_out_process should use ire_ipif->ipif_ill instead of ire_stq. Will
> this cause problems?
It shouldn't. You mean this bit of code in ipsec_out_process()?
/*
* Save the outbound ill index. When the packet comes back
* from IPsec, we make sure the ill hasn't changed or disappeared
* before sending it the accelerated packet.
*/
if ((ire != NULL) && (io->ipsec_out_capab_ill_index == 0)) {
int ifindex;
ill = ire_to_ill(ire);
ifindex = ill->ill_phyint->phyint_ifindex;
io->ipsec_out_capab_ill_index = ifindex;
}
I don't see why reworking how ill gets assigned will cause IPsec problems.
> (Also, did someone forget to initialize `ill' to NULL at the top of
> ipsec_out_process()?)
Someone did. And I probably missed it on the code review.
> Finally, related to all of the above: do you have suggestions on how I
> might go about sanity-testing IPsec after we agree upon the changes?
IKE-STC is your best friend for such testing. Seb's pretty familiar with it
(thanks to tunneling) and once you find two machines (I like mixed-endian
pairs) to run it, just configure, fire, and forget.
> [2] It's also sometimes used by ipsec_out_process() when calling into
> IPQoS's ip_process() routine, but that use doesn't matter much
> because we always map underlying interface indices to IPMP group
> interface indices when applying IPQoS policy. That said, it is
> confusing that ip_output_options() calls ipsec_out_process() with
> a third argument of ipsec_out_ill_index, given ipsec_out_process()
> always ignores that argument from that call site (because
> ipsec_out_proc_begin is nonzero). Why not just pass zero?
Good question.
If I didn't answer something clearly, please let me know!
Dan
