[PATCH] libiptc: don't set_changed() when checking rules with module jumps

[PATCH] libiptc: don't set_changed() when checking rules with module jumps

[Dan Williams]

---
 libiptc/libiptc.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index 2c66d04..a6e7057 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -1686,7 +1686,8 @@ iptcc_standard_map(struct rule_head *r, int verdict)
 
 static int
 iptcc_map_target(struct xtc_handle *const handle,
-	   struct rule_head *r)
+	   struct rule_head *r,
+	   bool dry_run)
 {
 	STRUCT_ENTRY *e = r->entry;
 	STRUCT_ENTRY_TARGET *t = GET_TARGET(e);
@@ -1731,7 +1732,8 @@ iptcc_map_target(struct xtc_handle *const handle,
 	       0,
 	       FUNCTION_MAXNAMELEN - 1 - strlen(t->u.user.name));
 	r->type = IPTCC_R_MODULE;
-	set_changed(handle);
+	if (!dry_run)
+		set_changed(handle);
 	return 1;
 }
 
@@ -1781,7 +1783,7 @@ TC_INSERT_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		free(r);
 		return 0;
 	}
@@ -1831,7 +1833,7 @@ TC_REPLACE_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		free(r);
 		return 0;
 	}
@@ -1870,7 +1872,7 @@ TC_APPEND_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
 		return 0;
@@ -1976,7 +1978,7 @@ static int delete_entry(const IPT_CHAINLABEL chain, const STRUCT_ENTRY *origfw,
 
 	memcpy(r->entry, origfw, origfw->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_NOMAP;
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, dry_run)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
 		return 0;
-- 
2.9.3

[PATCH v2] libiptc: don't set_changed() when checking rules with module jumps

[Dan Williams]

Signed-off-by: Dan Williams <dcbw@redhat.com>
---
** this time with a signed-off-by, no other changes.

 libiptc/libiptc.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index 2c66d04..a6e7057 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -1686,7 +1686,8 @@ iptcc_standard_map(struct rule_head *r, int verdict)
 
 static int
 iptcc_map_target(struct xtc_handle *const handle,
-	   struct rule_head *r)
+	   struct rule_head *r,
+	   bool dry_run)
 {
 	STRUCT_ENTRY *e = r->entry;
 	STRUCT_ENTRY_TARGET *t = GET_TARGET(e);
@@ -1731,7 +1732,8 @@ iptcc_map_target(struct xtc_handle *const handle,
 	       0,
 	       FUNCTION_MAXNAMELEN - 1 - strlen(t->u.user.name));
 	r->type = IPTCC_R_MODULE;
-	set_changed(handle);
+	if (!dry_run)
+		set_changed(handle);
 	return 1;
 }
 
@@ -1781,7 +1783,7 @@ TC_INSERT_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		free(r);
 		return 0;
 	}
@@ -1831,7 +1833,7 @@ TC_REPLACE_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		free(r);
 		return 0;
 	}
@@ -1870,7 +1872,7 @@ TC_APPEND_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
 		return 0;
@@ -1976,7 +1978,7 @@ static int delete_entry(const IPT_CHAINLABEL chain, const STRUCT_ENTRY *origfw,
 
 	memcpy(r->entry, origfw, origfw->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_NOMAP;
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, dry_run)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
 		return 0;
-- 
2.9.3

Re: [PATCH v2] libiptc: don't set_changed() when checking rules with module jumps

[Pablo Neira Ayuso]

On Fri, Feb 24, 2017 at 12:25:55PM -0600, Dan Williams wrote:
> Signed-off-by: Dan Williams <dcbw@redhat.com>

Please, add a description to this patch, explaining why we need this
and so on.

Thanks.

[PATCH v3] libiptc: don't set_changed() when checking rules with module jumps

[Dan Williams]

Checking a rule that includes a jump to a module-based target currently
sets the "changed" flag on the handle, which then causes TC_COMMIT() to
run through the whole SO_SET_REPLACE/SO_SET_ADD_COUNTERS path.  This
seems wrong for simply checking rules, an operation which is documented
as "...does not alter the existing iptables configuration..." but yet
it clearly could do so.

Fix that by ensuring that rule check operations for module targets
don't set the changed flag, and thus exit early from TC_COMMIT().

Signed-off-by: Dan Williams <dcbw@redhat.com>
---
 libiptc/libiptc.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/libiptc/libiptc.c b/libiptc/libiptc.c
index 2c66d04..a6e7057 100644
--- a/libiptc/libiptc.c
+++ b/libiptc/libiptc.c
@@ -1686,7 +1686,8 @@ iptcc_standard_map(struct rule_head *r, int verdict)
 
 static int
 iptcc_map_target(struct xtc_handle *const handle,
-	   struct rule_head *r)
+	   struct rule_head *r,
+	   bool dry_run)
 {
 	STRUCT_ENTRY *e = r->entry;
 	STRUCT_ENTRY_TARGET *t = GET_TARGET(e);
@@ -1731,7 +1732,8 @@ iptcc_map_target(struct xtc_handle *const handle,
 	       0,
 	       FUNCTION_MAXNAMELEN - 1 - strlen(t->u.user.name));
 	r->type = IPTCC_R_MODULE;
-	set_changed(handle);
+	if (!dry_run)
+		set_changed(handle);
 	return 1;
 }
 
@@ -1781,7 +1783,7 @@ TC_INSERT_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		free(r);
 		return 0;
 	}
@@ -1831,7 +1833,7 @@ TC_REPLACE_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		free(r);
 		return 0;
 	}
@@ -1870,7 +1872,7 @@ TC_APPEND_ENTRY(const IPT_CHAINLABEL chain,
 	memcpy(r->entry, e, e->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_SET;
 
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, false)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
 		return 0;
@@ -1976,7 +1978,7 @@ static int delete_entry(const IPT_CHAINLABEL chain, const STRUCT_ENTRY *origfw,
 
 	memcpy(r->entry, origfw, origfw->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_NOMAP;
-	if (!iptcc_map_target(handle, r)) {
+	if (!iptcc_map_target(handle, r, dry_run)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
 		return 0;
-- 
2.9.3

Re: [PATCH v3] libiptc: don't set_changed() when checking rules with module jumps

[Pablo Neira Ayuso]

On Sat, Feb 25, 2017 at 10:02:03PM -0600, Dan Williams wrote:
> Checking a rule that includes a jump to a module-based target currently
> sets the "changed" flag on the handle, which then causes TC_COMMIT() to
> run through the whole SO_SET_REPLACE/SO_SET_ADD_COUNTERS path.  This
> seems wrong for simply checking rules, an operation which is documented
> as "...does not alter the existing iptables configuration..." but yet
> it clearly could do so.
> 
> Fix that by ensuring that rule check operations for module targets
> don't set the changed flag, and thus exit early from TC_COMMIT().

Thanks for explaining. How are you hitting this problem? I'm curious
to see if I can reproduce it.

Thanks!

Re: [PATCH v3] libiptc: don't set_changed() when checking rules with module jumps

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Feb 25, 2017 at 10:02:03PM -0600, Dan Williams wrote:
> > Checking a rule that includes a jump to a module-based target currently
> > sets the "changed" flag on the handle, which then causes TC_COMMIT() to
> > run through the whole SO_SET_REPLACE/SO_SET_ADD_COUNTERS path.  This
> > seems wrong for simply checking rules, an operation which is documented
> > as "...does not alter the existing iptables configuration..." but yet
> > it clearly could do so.
> > 
> > Fix that by ensuring that rule check operations for module targets
> > don't set the changed flag, and thus exit early from TC_COMMIT().
> 
> Thanks for explaining. How are you hitting this problem? I'm curious
> to see if I can reproduce it.

Its easy to reproduce.

iptables -t nat -o lo -A POSTROUTING -s 10.2.3.4 -j MASQUERADE
iptables -t nat -o lo -C POSTROUTING -s 10.2.3.4 -j MASQUERADE

you should see (via strace) that 2nd command also issues the iptables
setsockopt calls.

Re: [PATCH v3] libiptc: don't set_changed() when checking rules with module jumps

[Pablo Neira Ayuso]

On Tue, Feb 28, 2017 at 01:03:07PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sat, Feb 25, 2017 at 10:02:03PM -0600, Dan Williams wrote:
> > > Checking a rule that includes a jump to a module-based target currently
> > > sets the "changed" flag on the handle, which then causes TC_COMMIT() to
> > > run through the whole SO_SET_REPLACE/SO_SET_ADD_COUNTERS path.  This
> > > seems wrong for simply checking rules, an operation which is documented
> > > as "...does not alter the existing iptables configuration..." but yet
> > > it clearly could do so.
> > > 
> > > Fix that by ensuring that rule check operations for module targets
> > > don't set the changed flag, and thus exit early from TC_COMMIT().
> > 
> > Thanks for explaining. How are you hitting this problem? I'm curious
> > to see if I can reproduce it.
> 
> Its easy to reproduce.
> 
> iptables -t nat -o lo -A POSTROUTING -s 10.2.3.4 -j MASQUERADE
> iptables -t nat -o lo -C POSTROUTING -s 10.2.3.4 -j MASQUERADE
> 
> you should see (via strace) that 2nd command also issues the iptables
> setsockopt calls.

Thanks for explain. Applied, thanks.