Dan,
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.
* 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.
* 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.
* 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?
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? (Also, did someone forget to initialize `ill' to
NULL at the top of ipsec_out_process()?)
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?
Thanks!
[1] In particular, the use in ip_newroute*() is gone, as is
ipsec_out_attach_if.
[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?
--
meem
