I don't know much about l2tp, pipex or npppd. So I cannot say if
the new logic is correct. But I guess you have tested that.
The tdb mutex and ref counting looks correct.
> + struct tdb *tdb, *tdblocal = NULL;
The variable names tdb and tdbp are used very inconsistently within
IPsec. Don't use both. I think tdpb and a tdbflow are sufficient.
> + if (ipsecflowinfo != 0)
> + ids = ipsp_ids_lookup(ipsecflowinfo);
Can you move that to the place where it is needed?
> + /*
> + * Prepare tdb for searching the correct SPD by rn_lookup().
> + * "tdb_filtemask" of the tdb is used to find the correct SPD when
> + * multiple policies are overlapped.
> + */
> + tdb = tdbp;
> + if (ipsecflowinfo != 0 && ids != NULL) {
> + KASSERT(tdbp == NULL);
> + KASSERT(direction == IPSP_DIRECTION_OUT);
> + if ((tdblocal = gettdbbyflow(rdomain, direction, &dst,
> + IPPROTO_ESP, ids)) != NULL)
> + tdb = tdblocal;
> + }
> +
> /* Actual SPD lookup. */
> - if ((rnh = spd_table_get(rdomain)) == NULL ||
> - (rn = rn_match((caddr_t)&dst, rnh)) == NULL) {
> + rnh = spd_table_get(rdomain);
> + if (rnh != NULL) {
> + if (tdb != NULL)
> + rn = rn_lookup((caddr_t)&tdb->tdb_filter,
> + (caddr_t)&tdb->tdb_filtermask, rnh);
> + else
> + rn = rn_match((caddr_t)&dst, rnh);
> + }
> + tdb_unref(tdblocal);
Perhaps it is easier to understand this way:
if (ipsecflowinfo != 0) {
ids = ipsp_ids_lookup(ipsecflowinfo);
if (ids != NULL)
tdbflow = gettdbbyflow(rdomain, direction, &dst,
IPPROTO_ESP, ids);
}
/* Actual SPD lookup. */
rnh = spd_table_get(rdomain);
if (rnh != NULL) {
if (tdbflow != NULL)
rn = rn_lookup((caddr_t)&tdbflow->tdb_filter,
(caddr_t)&tdbflow->tdb_filtermask, rnh);
else if (tdb != NULL)
rn = rn_lookup((caddr_t)&tdbp->tdb_filter,
(caddr_t)&tdbp->tdb_filtermask, rnh);
else
rn = rn_match((caddr_t)&dst, rnh);
}
tdb_unref(tdbflow);
It is hard to say whether the new
rn_lookup(tdbp->tdb_filter/tdbp->tdb_filtermask) changes existing
IPsec behavior for setups without l2tp. Do we need it there?
I never ran into problems patching the correct policy.
bluhm
