* [PATCH] libiptc: don't set_changed() when checking rules with module jumps
@ 2017-02-24 18:15 Dan Williams
2017-02-24 18:25 ` [PATCH v2] " Dan Williams
0 siblings, 1 reply; 7+ messages in thread
From: Dan Williams @ 2017-02-24 18:15 UTC (permalink / raw)
To: netfilter-devel
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2] libiptc: don't set_changed() when checking rules with module jumps
2017-02-24 18:15 [PATCH] libiptc: don't set_changed() when checking rules with module jumps Dan Williams
@ 2017-02-24 18:25 ` Dan Williams
2017-02-25 10:59 ` Pablo Neira Ayuso
2017-02-26 4:02 ` [PATCH v3] " Dan Williams
0 siblings, 2 replies; 7+ messages in thread
From: Dan Williams @ 2017-02-24 18:25 UTC (permalink / raw)
To: netfilter-devel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] libiptc: don't set_changed() when checking rules with module jumps
2017-02-24 18:25 ` [PATCH v2] " Dan Williams
@ 2017-02-25 10:59 ` Pablo Neira Ayuso
2017-02-26 4:02 ` [PATCH v3] " Dan Williams
1 sibling, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-25 10:59 UTC (permalink / raw)
To: Dan Williams; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] libiptc: don't set_changed() when checking rules with module jumps
2017-02-24 18:25 ` [PATCH v2] " Dan Williams
2017-02-25 10:59 ` Pablo Neira Ayuso
@ 2017-02-26 4:02 ` Dan Williams
2017-02-28 11:33 ` Pablo Neira Ayuso
1 sibling, 1 reply; 7+ messages in thread
From: Dan Williams @ 2017-02-26 4:02 UTC (permalink / raw)
To: netfilter-devel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3] libiptc: don't set_changed() when checking rules with module jumps
2017-02-26 4:02 ` [PATCH v3] " Dan Williams
@ 2017-02-28 11:33 ` Pablo Neira Ayuso
2017-02-28 12:03 ` Florian Westphal
0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-28 11:33 UTC (permalink / raw)
To: Dan Williams; +Cc: netfilter-devel
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] libiptc: don't set_changed() when checking rules with module jumps
2017-02-28 11:33 ` Pablo Neira Ayuso
@ 2017-02-28 12:03 ` Florian Westphal
2017-02-28 12:18 ` Pablo Neira Ayuso
0 siblings, 1 reply; 7+ messages in thread
From: Florian Westphal @ 2017-02-28 12:03 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: Dan Williams, netfilter-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] libiptc: don't set_changed() when checking rules with module jumps
2017-02-28 12:03 ` Florian Westphal
@ 2017-02-28 12:18 ` Pablo Neira Ayuso
0 siblings, 0 replies; 7+ messages in thread
From: Pablo Neira Ayuso @ 2017-02-28 12:18 UTC (permalink / raw)
To: Florian Westphal; +Cc: Dan Williams, netfilter-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-02-28 12:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-24 18:15 [PATCH] libiptc: don't set_changed() when checking rules with module jumps Dan Williams
2017-02-24 18:25 ` [PATCH v2] " Dan Williams
2017-02-25 10:59 ` Pablo Neira Ayuso
2017-02-26 4:02 ` [PATCH v3] " Dan Williams
2017-02-28 11:33 ` Pablo Neira Ayuso
2017-02-28 12:03 ` Florian Westphal
2017-02-28 12:18 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).