[Swan-dev] xfrmi work conflict
Antony Antony
antony at phenome.org
Fri Sep 18 14:47:12 UTC 2020
On Fri, Sep 18, 2020 at 09:23:33AM -0400, Paul Wouters wrote:
> On Thu, 17 Sep 2020, Antony Antony wrote:
>
> > recent xfrmi changes
> > https://github.com/libreswan/libreswan/commit/78253c41f6200f2f505e14775cdbaca3b40ae5c8
> > has a few conflicts with xfrmi fixes I was working on, and discused here on
> > swan-dev. I am not able to follow up the code churn and things going too
> > fast, may be there is pressure of release. I was hopping merge soon, been
> > working for a few weeks.
> >
> > https://github.com/antonyantony/libreswan/tree/xfrmi-outmark
> > If any one interested or pick up bits of it here it as-is.
>
> I have picked these up and merged it.
>
> Note that ikev2-xfrmi-14-fwmark still fails. I think you expect that
> because the test is in WIP. The fwmark that is commited in test output
> is fwmark 0x1000000 but the test actually only uses fwmark 0x1. So I am
> not sure what the idea is.
from a quick look add
https://github.com/antonyantony/libreswan/commit/799dfa935aef3d366021bfea6004766359921b1e
I think you missed this commit. May be the commit message was misleading.
>
> I've tested compiling with and without USE_XFRM_INTERFACE. Please let me
> know if you see any issues.
Hi Paul,
thanks for the quick merge.
I was getting confused with too many rapid changes in the code and not able
to catch up with testruns on my branch.
I have an UNTESTED fix to the merge and your commit f965623b7. See the
attached patch. I am too shocked to test it at the moment. May be I will
test it later! May be there are more places xfrm_if_id zero is assumed "no"
that need fixing. In hindsight I should have used yn_no instead of 0 every
where!
A minor nit on commit meesages. git object reference url in commit message
will disappear and commit message would look confusing. I discourage
transient url in commit messsage for the future.
I also have non echnical issue with f965623b7. I am personally shocked by
this commit.
I feel as it is PLUTO_XFRMI_REMAP_IF_ID_ZERO is a horrible hack and my
predication is it is more pain than what it is worth.
To me this commit and the merge look poorly tested. May be there is a need
for quick resolution under time pressure. That is why I am sending untested
patch for review and further testing than commiting it.
Now the commit f965623b7 is in main I feel the table turned around. I have
to do the work disproving while I feel you should have tested more and/or
asked feedback before commiting it.
In an ideal world my preference is revert f965623b7, take closer look at
supporting ipsec0, add test cases discuss it before adding support for
ipsec0. Git revert may be harder now, atleast revert that feture!
regards,
-antony
-------------- next part --------------
>From 8a542e00a41de67eb8c613f866ad1d33401a7994 Mon Sep 17 00:00:00 2001
From: Antony Antony <antony at phenome.org>
Date: Fri, 18 Sep 2020 14:11:36 +0000
Subject: [PATCH] pluto: use yn_no not 0
Fixes: c7fe6e85e8 ("pluto: xfrmi used mark-out for XFRMA_SET_MARK")
---
programs/pluto/ikev1_aggr.c | 4 ++--
programs/pluto/ikev1_main.c | 4 ++--
programs/pluto/ikev1_quick.c | 6 +++---
programs/pluto/ikev2_child.c | 2 +-
programs/pluto/ikev2_parent.c | 2 +-
programs/pluto/kernel_xfrm.c | 12 ++++++------
programs/pluto/kernel_xfrm_interface.c | 2 +-
7 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/programs/pluto/ikev1_aggr.c b/programs/pluto/ikev1_aggr.c
index e5f7c31de6..42a0454b77 100644
--- a/programs/pluto/ikev1_aggr.c
+++ b/programs/pluto/ikev1_aggr.c
@@ -834,7 +834,7 @@ static stf_status aggr_inR1_outI2_tail(struct msg_digest *md)
IKE_SA_established(pexpect_ike_sa(st));
#ifdef USE_XFRM_INTERFACE
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, st->st_logger))
return STF_FATAL;
#endif
@@ -960,7 +960,7 @@ stf_status aggr_inI2(struct state *st, struct msg_digest *md)
IKE_SA_established(pexpect_ike_sa(st));
#ifdef USE_XFRM_INTERFACE
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, st->st_logger))
return STF_FATAL;
#endif
diff --git a/programs/pluto/ikev1_main.c b/programs/pluto/ikev1_main.c
index 7008706a08..596cba348b 100644
--- a/programs/pluto/ikev1_main.c
+++ b/programs/pluto/ikev1_main.c
@@ -1714,7 +1714,7 @@ stf_status main_inI3_outR3(struct state *st, struct msg_digest *md)
IKE_SA_established(pexpect_ike_sa(st));
#ifdef USE_XFRM_INTERFACE
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, st->st_logger))
return STF_FATAL;
#endif
@@ -1769,7 +1769,7 @@ stf_status main_inR3(struct state *st, struct msg_digest *md)
IKE_SA_established(pexpect_ike_sa(st));
#ifdef USE_XFRM_INTERFACE
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, st->st_logger))
return STF_FATAL;
#endif
diff --git a/programs/pluto/ikev1_quick.c b/programs/pluto/ikev1_quick.c
index b220672dee..ac81f4de0d 100644
--- a/programs/pluto/ikev1_quick.c
+++ b/programs/pluto/ikev1_quick.c
@@ -1490,7 +1490,7 @@ static stf_status quick_inI1_outR1_continue12_tail(struct msg_digest *md,
*/
#ifdef USE_XFRM_INTERFACE
struct connection *c = st->st_connection;
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, st->st_logger))
return STF_FATAL;
#endif
@@ -1720,7 +1720,7 @@ stf_status quick_inR1_outI2_tail(struct msg_digest *md,
* failure won't look like success.
*/
#ifdef USE_XFRM_INTERFACE
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, st->st_logger))
return STF_FATAL;
#endif
@@ -1759,7 +1759,7 @@ stf_status quick_inI2(struct state *st, struct msg_digest *md UNUSED)
*/
#ifdef USE_XFRM_INTERFACE
struct connection *c = st->st_connection;
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, st->st_logger))
return STF_FATAL;
#endif
diff --git a/programs/pluto/ikev2_child.c b/programs/pluto/ikev2_child.c
index d258d258fd..fd04c46970 100644
--- a/programs/pluto/ikev2_child.c
+++ b/programs/pluto/ikev2_child.c
@@ -309,7 +309,7 @@ stf_status ikev2_child_sa_respond(struct ike_sa *ike,
ike->sa.st_connection->newest_isakmp_sa = ike->sa.st_serialno;
} else {
#ifdef USE_XFRM_INTERFACE
- if (c->xfrmi != NULL && c->xfrmi->if_id != 0)
+ if (c->xfrmi != NULL && c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, child->sa.st_logger))
return STF_FATAL;
#endif
diff --git a/programs/pluto/ikev2_parent.c b/programs/pluto/ikev2_parent.c
index 08629cb700..41ac78480b 100644
--- a/programs/pluto/ikev2_parent.c
+++ b/programs/pluto/ikev2_parent.c
@@ -3784,7 +3784,7 @@ static stf_status ikev2_process_ts_and_rest(struct msg_digest *md)
/* before calling do_command() */
if (st->st_state->kind != STATE_V2_REKEY_CHILD_I1)
if (c->xfrmi != NULL &&
- c->xfrmi->if_id != 0)
+ c->xfrmi->if_id != yn_no)
if (add_xfrmi(c, child->sa.st_logger))
return STF_FATAL;
#endif
diff --git a/programs/pluto/kernel_xfrm.c b/programs/pluto/kernel_xfrm.c
index a5dda3b2ae..1e2868a89c 100644
--- a/programs/pluto/kernel_xfrm.c
+++ b/programs/pluto/kernel_xfrm.c
@@ -718,7 +718,7 @@ static bool netlink_raw_eroute(const ip_address *this_host,
{
struct sa_mark sa_mark = (dir == XFRM_POLICY_IN) ? sa_marks->in : sa_marks->out;
- if (sa_mark.val != 0 && sa_mark.mask != 0 && xfrm_if_id == 0) {
+ if (sa_mark.val != 0 && sa_mark.mask != 0 && xfrm_if_id == yn_no) {
struct xfrm_mark xfrm_mark;
struct rtattr* mark_attr;
@@ -733,7 +733,7 @@ static bool netlink_raw_eroute(const ip_address *this_host,
}
}
#ifdef USE_XFRM_INTERFACE
- if (xfrm_if_id != 0) {
+ if (xfrm_if_id != yn_no) {
dbg("%s netlink: XFRMA_IF_ID %" PRIu32 " req.n.nlmsg_type=%" PRIu32,
__func__, xfrm_if_id, req.n.nlmsg_type);
nl_addattr32(&req.n, sizeof(req.data), XFRMA_IF_ID, xfrm_if_id);
@@ -1543,7 +1543,7 @@ static bool netlink_add_sa(const struct kernel_sa *sa, bool replace)
}
#ifdef USE_XFRM_INTERFACE
- if (sa->xfrm_if_id != 0) {
+ if (sa->xfrm_if_id != yn_no) {
dbg("%s netlink: XFRMA_IF_ID %" PRIu32 " req.n.nlmsg_type=%" PRIu32,
__func__, sa->xfrm_if_id, req.n.nlmsg_type);
nl_addattr32(&req.n, sizeof(req.data), XFRMA_IF_ID, sa->xfrm_if_id);
@@ -2161,7 +2161,7 @@ static bool netlink_sag_eroute(const struct state *st, const struct spd_route *s
ENCAPSULATION_MODE_TRANSPORT;
}
- uint32_t xfrm_if_id = c->xfrmi != NULL ? c->xfrmi->if_id : 0;
+ uint32_t xfrm_if_id = c->xfrmi != NULL ? c->xfrmi->if_id : yn_no;
return eroute_connection(sr, inner_spi, inner_spi, inner_proto,
inner_esatype, proto_info + i,
@@ -2305,7 +2305,7 @@ static bool netlink_shunt_eroute(const struct connection *c,
deltatime(0),
calculate_sa_prio(c, FALSE),
&c->sa_marks,
- 0 /* xfrm_if_id needed for shunt? */,
+ yn_no /* xfrm_if_id needed for shunt? */,
op, buf2,
c->policy_label))
return FALSE;
@@ -2333,7 +2333,7 @@ static bool netlink_shunt_eroute(const struct connection *c,
deltatime(0),
calculate_sa_prio(c, FALSE),
&c->sa_marks,
- 0, /* xfrm_if_id needed for shunt? */
+ yn_no, /* xfrm_if_id needed for shunt? */
op, buf2,
c->policy_label);
}
diff --git a/programs/pluto/kernel_xfrm_interface.c b/programs/pluto/kernel_xfrm_interface.c
index e2633e4e11..708fbc01d4 100644
--- a/programs/pluto/kernel_xfrm_interface.c
+++ b/programs/pluto/kernel_xfrm_interface.c
@@ -722,7 +722,7 @@ void stale_xfrmi_interfaces(void)
snprintf(if_name, sizeof(if_name), XFRMI_DEV_FORMAT, IPSEC1_XFRM_IF_ID); /* first one ipsec1 */
unsigned int if_id = if_nametoindex(if_name);
- if (if_id != 0) {
+ if (if_id != yn_no) {
loglog(RC_LOG_SERIOUS, "found an unexpected interface %s if_id=%u From previous pluto run?",
if_name, if_id);
return; /* ERROR */
--
2.21.3
More information about the Swan-dev
mailing list