diff mbox

[iptables] extensions: libxt_conntrack: Fix 'state' translation to nft

Message ID 20170307153507.14640-1-phil@nwl.cc
State Not Applicable
Delegated to: Pablo Neira
Headers show

Commit Message

Phil Sutter March 7, 2017, 3:35 p.m. UTC
While translating a conntrack state match in old syntax, matches are
looked up by name, only. This returned the revision 0 entry since
matches are registered in reverse order of appearance in the array
passed to xtables_register_matches(). The problem is that revision 0
doesn't define an xlate callback.

Fix this by reordering the matches in conntrack_mt_reg so that the
highest revision one is found first.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
The strange thing here is that I'm pretty sure this has been working
once. My logs from playing with iptables-restore-translate from November
2016 indicate that. Yet I have not been able to find a point in iptables
git history in which it works.
---
 extensions/libxt_conntrack.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comments

Pablo Neira Ayuso March 7, 2017, 4:17 p.m. UTC | #1
On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> While translating a conntrack state match in old syntax, matches are
> looked up by name, only. This returned the revision 0 entry since
> matches are registered in reverse order of appearance in the array
> passed to xtables_register_matches(). The problem is that revision 0
> doesn't define an xlate callback.
> 
> Fix this by reordering the matches in conntrack_mt_reg so that the
> highest revision one is found first.

Applied, thanks Phil.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso March 7, 2017, 4:20 p.m. UTC | #2
On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > While translating a conntrack state match in old syntax, matches are
> > looked up by name, only. This returned the revision 0 entry since
> > matches are registered in reverse order of appearance in the array
> > passed to xtables_register_matches(). The problem is that revision 0
> > doesn't define an xlate callback.
> > 
> > Fix this by reordering the matches in conntrack_mt_reg so that the
> > highest revision one is found first.
> 
> Applied, thanks Phil.

Wait.

Do you mean this case?

# iptables-translate -I INPUT -m state --state NEW
nft insert rule ip filter INPUT ct state new  counter

Hm, this works here.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Sutter March 7, 2017, 4:54 p.m. UTC | #3
On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > While translating a conntrack state match in old syntax, matches are
> > > looked up by name, only. This returned the revision 0 entry since
> > > matches are registered in reverse order of appearance in the array
> > > passed to xtables_register_matches(). The problem is that revision 0
> > > doesn't define an xlate callback.
> > > 
> > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > highest revision one is found first.
> > 
> > Applied, thanks Phil.
> 
> Wait.
> 
> Do you mean this case?
> 
> # iptables-translate -I INPUT -m state --state NEW
> nft insert rule ip filter INPUT ct state new  counter
> 
> Hm, this works here.

Yes, that's it. And it working for you just emphasizes something's fishy
here. I just reverted my patch, then it's like this:

| $ ./configure --prefix=$PWD/install && make && make install
| [...]
| $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
| nft # -I INPUT -m state --state NEW

Using 'strace -eopen' I see that libxt_state.so from the install
destination is used, not the system one.

Also, I did this change for debugging:

| --- a/iptables/xtables-translate.c
| +++ b/iptables/xtables-translate.c
| @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
|                         .escape_quotes  = !cs->restore,
|                 };
|  
| +               printf("found match %s, rev %u, xlate is %p\n",
| +                               matchp->match->name,
| +                               matchp->match->revision,
| +                               matchp->match->xlate);
|                 if (!matchp->match->xlate)
|                         return 0;

The output is then:

| $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
| nft found match state, rev 0, xlate is (nil)
| # -I INPUT -m state --state NEW

Am I missing something??

Thanks, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso March 7, 2017, 7:31 p.m. UTC | #4
On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > While translating a conntrack state match in old syntax, matches are
> > > > looked up by name, only. This returned the revision 0 entry since
> > > > matches are registered in reverse order of appearance in the array
> > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > doesn't define an xlate callback.
> > > > 
> > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > highest revision one is found first.
> > > 
> > > Applied, thanks Phil.
> > 
> > Wait.
> > 
> > Do you mean this case?
> > 
> > # iptables-translate -I INPUT -m state --state NEW
> > nft insert rule ip filter INPUT ct state new  counter
> > 
> > Hm, this works here.
> 
> Yes, that's it. And it working for you just emphasizes something's fishy
> here. I just reverted my patch, then it's like this:
> 
> | $ ./configure --prefix=$PWD/install && make && make install
> | [...]
> | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> | nft # -I INPUT -m state --state NEW
> 
> Using 'strace -eopen' I see that libxt_state.so from the install
> destination is used, not the system one.
> 
> Also, I did this change for debugging:
> 
> | --- a/iptables/xtables-translate.c
> | +++ b/iptables/xtables-translate.c
> | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> |                         .escape_quotes  = !cs->restore,
> |                 };
> |  
> | +               printf("found match %s, rev %u, xlate is %p\n",
> | +                               matchp->match->name,
> | +                               matchp->match->revision,
> | +                               matchp->match->xlate);
> |                 if (!matchp->match->xlate)
> |                         return 0;
> 
> The output is then:
> 
> | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> | nft found match state, rev 0, xlate is (nil)
> | # -I INPUT -m state --state NEW
> 
> Am I missing something??

Via nft_compatible_revision() I'm getting revision 3. Are you using
latest kernel from git?
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Sutter March 7, 2017, 8:07 p.m. UTC | #5
On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > While translating a conntrack state match in old syntax, matches are
> > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > matches are registered in reverse order of appearance in the array
> > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > doesn't define an xlate callback.
> > > > > 
> > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > highest revision one is found first.
> > > > 
> > > > Applied, thanks Phil.
> > > 
> > > Wait.
> > > 
> > > Do you mean this case?
> > > 
> > > # iptables-translate -I INPUT -m state --state NEW
> > > nft insert rule ip filter INPUT ct state new  counter
> > > 
> > > Hm, this works here.
> > 
> > Yes, that's it. And it working for you just emphasizes something's fishy
> > here. I just reverted my patch, then it's like this:
> > 
> > | $ ./configure --prefix=$PWD/install && make && make install
> > | [...]
> > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > | nft # -I INPUT -m state --state NEW
> > 
> > Using 'strace -eopen' I see that libxt_state.so from the install
> > destination is used, not the system one.
> > 
> > Also, I did this change for debugging:
> > 
> > | --- a/iptables/xtables-translate.c
> > | +++ b/iptables/xtables-translate.c
> > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > |                         .escape_quotes  = !cs->restore,
> > |                 };
> > |  
> > | +               printf("found match %s, rev %u, xlate is %p\n",
> > | +                               matchp->match->name,
> > | +                               matchp->match->revision,
> > | +                               matchp->match->xlate);
> > |                 if (!matchp->match->xlate)
> > |                         return 0;
> > 
> > The output is then:
> > 
> > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > | nft found match state, rev 0, xlate is (nil)
> > | # -I INPUT -m state --state NEW
> > 
> > Am I missing something??
> 
> Via nft_compatible_revision() I'm getting revision 3. Are you using
> latest kernel from git?

No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
installed, is that relevant?

Thanks, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso March 8, 2017, 10:36 a.m. UTC | #6
On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > matches are registered in reverse order of appearance in the array
> > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > doesn't define an xlate callback.
> > > > > > 
> > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > highest revision one is found first.
> > > > > 
> > > > > Applied, thanks Phil.
> > > > 
> > > > Wait.
> > > > 
> > > > Do you mean this case?
> > > > 
> > > > # iptables-translate -I INPUT -m state --state NEW
> > > > nft insert rule ip filter INPUT ct state new  counter
> > > > 
> > > > Hm, this works here.
> > > 
> > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > here. I just reverted my patch, then it's like this:
> > > 
> > > | $ ./configure --prefix=$PWD/install && make && make install
> > > | [...]
> > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > | nft # -I INPUT -m state --state NEW
> > > 
> > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > destination is used, not the system one.
> > > 
> > > Also, I did this change for debugging:
> > > 
> > > | --- a/iptables/xtables-translate.c
> > > | +++ b/iptables/xtables-translate.c
> > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > |                         .escape_quotes  = !cs->restore,
> > > |                 };
> > > |  
> > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > | +                               matchp->match->name,
> > > | +                               matchp->match->revision,
> > > | +                               matchp->match->xlate);
> > > |                 if (!matchp->match->xlate)
> > > |                         return 0;
> > > 
> > > The output is then:
> > > 
> > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > | nft found match state, rev 0, xlate is (nil)
> > > | # -I INPUT -m state --state NEW
> > > 
> > > Am I missing something??
> > 
> > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > latest kernel from git?
> 
> No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> installed, is that relevant?

I have a way to trigger this here, but not sure if that is the problem
there. If the kernel comes with no nft_compat support, then
nft_compatible_revision() fails, and
xtables_fully_register_pending_match() ends up selecting revision 0.

In such case, I can reproduce your problem.

# iptables-translate -A INPUT -m state --state NEW
nft # -A INPUT -m state --state NEW 
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Sutter March 8, 2017, 12:31 p.m. UTC | #7
On Wed, Mar 08, 2017 at 11:36:52AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> > On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > > matches are registered in reverse order of appearance in the array
> > > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > > doesn't define an xlate callback.
> > > > > > > 
> > > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > > highest revision one is found first.
> > > > > > 
> > > > > > Applied, thanks Phil.
> > > > > 
> > > > > Wait.
> > > > > 
> > > > > Do you mean this case?
> > > > > 
> > > > > # iptables-translate -I INPUT -m state --state NEW
> > > > > nft insert rule ip filter INPUT ct state new  counter
> > > > > 
> > > > > Hm, this works here.
> > > > 
> > > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > > here. I just reverted my patch, then it's like this:
> > > > 
> > > > | $ ./configure --prefix=$PWD/install && make && make install
> > > > | [...]
> > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > | nft # -I INPUT -m state --state NEW
> > > > 
> > > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > > destination is used, not the system one.
> > > > 
> > > > Also, I did this change for debugging:
> > > > 
> > > > | --- a/iptables/xtables-translate.c
> > > > | +++ b/iptables/xtables-translate.c
> > > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > > |                         .escape_quotes  = !cs->restore,
> > > > |                 };
> > > > |  
> > > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > > | +                               matchp->match->name,
> > > > | +                               matchp->match->revision,
> > > > | +                               matchp->match->xlate);
> > > > |                 if (!matchp->match->xlate)
> > > > |                         return 0;
> > > > 
> > > > The output is then:
> > > > 
> > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > | nft found match state, rev 0, xlate is (nil)
> > > > | # -I INPUT -m state --state NEW
> > > > 
> > > > Am I missing something??
> > > 
> > > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > > latest kernel from git?
> > 
> > No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> > from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> > installed, is that relevant?
> 
> I have a way to trigger this here, but not sure if that is the problem
> there. If the kernel comes with no nft_compat support, then
> nft_compatible_revision() fails, and
> xtables_fully_register_pending_match() ends up selecting revision 0.
> 
> In such case, I can reproduce your problem.
> 
> # iptables-translate -A INPUT -m state --state NEW
> nft # -A INPUT -m state --state NEW 

Ah! On my system, nft_compat is built as module and it didn't matter
whether I have it loaded or not, nft_compatible_revision() would never
succeed.

Oh man, I just found the cause: I was running iptables-translate as
unprivileged user. Calling it with sudo magically makes everything work.

I'll have a look whether it's possible to communicate the received
-EPERM back to the user.

Cheers, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso March 8, 2017, 1:38 p.m. UTC | #8
On Wed, Mar 08, 2017 at 01:31:51PM +0100, Phil Sutter wrote:
> On Wed, Mar 08, 2017 at 11:36:52AM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> > > On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > > > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > > > matches are registered in reverse order of appearance in the array
> > > > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > > > doesn't define an xlate callback.
> > > > > > > > 
> > > > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > > > highest revision one is found first.
> > > > > > > 
> > > > > > > Applied, thanks Phil.
> > > > > > 
> > > > > > Wait.
> > > > > > 
> > > > > > Do you mean this case?
> > > > > > 
> > > > > > # iptables-translate -I INPUT -m state --state NEW
> > > > > > nft insert rule ip filter INPUT ct state new  counter
> > > > > > 
> > > > > > Hm, this works here.
> > > > > 
> > > > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > > > here. I just reverted my patch, then it's like this:
> > > > > 
> > > > > | $ ./configure --prefix=$PWD/install && make && make install
> > > > > | [...]
> > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > | nft # -I INPUT -m state --state NEW
> > > > > 
> > > > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > > > destination is used, not the system one.
> > > > > 
> > > > > Also, I did this change for debugging:
> > > > > 
> > > > > | --- a/iptables/xtables-translate.c
> > > > > | +++ b/iptables/xtables-translate.c
> > > > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > > > |                         .escape_quotes  = !cs->restore,
> > > > > |                 };
> > > > > |  
> > > > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > > > | +                               matchp->match->name,
> > > > > | +                               matchp->match->revision,
> > > > > | +                               matchp->match->xlate);
> > > > > |                 if (!matchp->match->xlate)
> > > > > |                         return 0;
> > > > > 
> > > > > The output is then:
> > > > > 
> > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > | nft found match state, rev 0, xlate is (nil)
> > > > > | # -I INPUT -m state --state NEW
> > > > > 
> > > > > Am I missing something??
> > > > 
> > > > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > > > latest kernel from git?
> > > 
> > > No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> > > from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> > > installed, is that relevant?
> > 
> > I have a way to trigger this here, but not sure if that is the problem
> > there. If the kernel comes with no nft_compat support, then
> > nft_compatible_revision() fails, and
> > xtables_fully_register_pending_match() ends up selecting revision 0.
> > 
> > In such case, I can reproduce your problem.
> > 
> > # iptables-translate -A INPUT -m state --state NEW
> > nft # -A INPUT -m state --state NEW 
> 
> Ah! On my system, nft_compat is built as module and it didn't matter
> whether I have it loaded or not, nft_compatible_revision() would never
> succeed.
> 
> Oh man, I just found the cause: I was running iptables-translate as
> unprivileged user. Calling it with sudo magically makes everything work.
> 
> I'll have a look whether it's possible to communicate the received
> -EPERM back to the user.

I wonder if we just update xtables_globals for xtables-translate.c so
we don't require any kernel intervention, just add a dummy
nft_compatible_revision() that simply says: Yes, this revision is
good. So we just let libxtables select the largest revision number all
the time and we skip traffic and dependencies with the kernel.

It would be great if you can have a look into this. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Sutter March 8, 2017, 2:02 p.m. UTC | #9
On Wed, Mar 08, 2017 at 01:31:51PM +0100, Phil Sutter wrote:
> Oh man, I just found the cause: I was running iptables-translate as
> unprivileged user. Calling it with sudo magically makes everything work.
> 
> I'll have a look whether it's possible to communicate the received
> -EPERM back to the user.

I wonder how iptables should deal with this situation - in a regular
use-case, I guess the program will abort eventually anyway but for
iptables-translate it shouldn't really matter. So do you think it's OK
to make nft_compatible_revision() return 0 if it made it past
mnl_cb_run() and errno is EPERM?

Ideally it should warn as well, of course.

Cheers, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Sutter March 8, 2017, 2:03 p.m. UTC | #10
On Wed, Mar 08, 2017 at 02:38:42PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Mar 08, 2017 at 01:31:51PM +0100, Phil Sutter wrote:
> > On Wed, Mar 08, 2017 at 11:36:52AM +0100, Pablo Neira Ayuso wrote:
> > > On Tue, Mar 07, 2017 at 09:07:45PM +0100, Phil Sutter wrote:
> > > > On Tue, Mar 07, 2017 at 08:31:58PM +0100, Pablo Neira Ayuso wrote:
> > > > > On Tue, Mar 07, 2017 at 05:54:09PM +0100, Phil Sutter wrote:
> > > > > > On Tue, Mar 07, 2017 at 05:20:55PM +0100, Pablo Neira Ayuso wrote:
> > > > > > > On Tue, Mar 07, 2017 at 05:17:29PM +0100, Pablo Neira Ayuso wrote:
> > > > > > > > On Tue, Mar 07, 2017 at 04:35:07PM +0100, Phil Sutter wrote:
> > > > > > > > > While translating a conntrack state match in old syntax, matches are
> > > > > > > > > looked up by name, only. This returned the revision 0 entry since
> > > > > > > > > matches are registered in reverse order of appearance in the array
> > > > > > > > > passed to xtables_register_matches(). The problem is that revision 0
> > > > > > > > > doesn't define an xlate callback.
> > > > > > > > > 
> > > > > > > > > Fix this by reordering the matches in conntrack_mt_reg so that the
> > > > > > > > > highest revision one is found first.
> > > > > > > > 
> > > > > > > > Applied, thanks Phil.
> > > > > > > 
> > > > > > > Wait.
> > > > > > > 
> > > > > > > Do you mean this case?
> > > > > > > 
> > > > > > > # iptables-translate -I INPUT -m state --state NEW
> > > > > > > nft insert rule ip filter INPUT ct state new  counter
> > > > > > > 
> > > > > > > Hm, this works here.
> > > > > > 
> > > > > > Yes, that's it. And it working for you just emphasizes something's fishy
> > > > > > here. I just reverted my patch, then it's like this:
> > > > > > 
> > > > > > | $ ./configure --prefix=$PWD/install && make && make install
> > > > > > | [...]
> > > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > > | nft # -I INPUT -m state --state NEW
> > > > > > 
> > > > > > Using 'strace -eopen' I see that libxt_state.so from the install
> > > > > > destination is used, not the system one.
> > > > > > 
> > > > > > Also, I did this change for debugging:
> > > > > > 
> > > > > > | --- a/iptables/xtables-translate.c
> > > > > > | +++ b/iptables/xtables-translate.c
> > > > > > | @@ -100,6 +100,10 @@ int xlate_matches(const struct iptables_command_state *cs, struct xt_xlate *xl)
> > > > > > |                         .escape_quotes  = !cs->restore,
> > > > > > |                 };
> > > > > > |  
> > > > > > | +               printf("found match %s, rev %u, xlate is %p\n",
> > > > > > | +                               matchp->match->name,
> > > > > > | +                               matchp->match->revision,
> > > > > > | +                               matchp->match->xlate);
> > > > > > |                 if (!matchp->match->xlate)
> > > > > > |                         return 0;
> > > > > > 
> > > > > > The output is then:
> > > > > > 
> > > > > > | $ ./install/sbin/iptables-translate -I INPUT -m state --state NEW
> > > > > > | nft found match state, rev 0, xlate is (nil)
> > > > > > | # -I INPUT -m state --state NEW
> > > > > > 
> > > > > > Am I missing something??
> > > > > 
> > > > > Via nft_compatible_revision() I'm getting revision 3. Are you using
> > > > > latest kernel from git?
> > > > 
> > > > No, I'm running 4.9.10-gentoo here. But a quick test with nf-next kernel
> > > > from Feb 23rd in a VM shows the same result. I have libnftnl-1.0.6
> > > > installed, is that relevant?
> > > 
> > > I have a way to trigger this here, but not sure if that is the problem
> > > there. If the kernel comes with no nft_compat support, then
> > > nft_compatible_revision() fails, and
> > > xtables_fully_register_pending_match() ends up selecting revision 0.
> > > 
> > > In such case, I can reproduce your problem.
> > > 
> > > # iptables-translate -A INPUT -m state --state NEW
> > > nft # -A INPUT -m state --state NEW 
> > 
> > Ah! On my system, nft_compat is built as module and it didn't matter
> > whether I have it loaded or not, nft_compatible_revision() would never
> > succeed.
> > 
> > Oh man, I just found the cause: I was running iptables-translate as
> > unprivileged user. Calling it with sudo magically makes everything work.
> > 
> > I'll have a look whether it's possible to communicate the received
> > -EPERM back to the user.
> 
> I wonder if we just update xtables_globals for xtables-translate.c so
> we don't require any kernel intervention, just add a dummy
> nft_compatible_revision() that simply says: Yes, this revision is
> good. So we just let libxtables select the largest revision number all
> the time and we skip traffic and dependencies with the kernel.

Oh, that sounds like a great solution! I'll have a look.

Thanks, Phil
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/extensions/libxt_conntrack.c b/extensions/libxt_conntrack.c
index 72c522004a7ea..60ce9d1dc0a2e 100644
--- a/extensions/libxt_conntrack.c
+++ b/extensions/libxt_conntrack.c
@@ -1507,6 +1507,19 @@  static struct xtables_match conntrack_mt_reg[] = {
 	{
 		.family        = NFPROTO_UNSPEC,
 		.name          = "state",
+		.revision      = 0,
+		.version       = XTABLES_VERSION,
+		.size          = XT_ALIGN(sizeof(struct xt_state_info)),
+		.userspacesize = XT_ALIGN(sizeof(struct xt_state_info)),
+		.help          = state_help,
+		.print         = state_print,
+		.save          = state_save,
+		.x6_parse      = state_parse,
+		.x6_options    = state_opts,
+	},
+	{
+		.family        = NFPROTO_UNSPEC,
+		.name          = "state",
 		.real_name     = "conntrack",
 		.revision      = 1,
 		.ext_flags     = XTABLES_EXT_ALIAS,
@@ -1550,19 +1563,6 @@  static struct xtables_match conntrack_mt_reg[] = {
 		.x6_options    = state_opts,
 		.xlate         = state_xlate,
 	},
-	{
-		.family        = NFPROTO_UNSPEC,
-		.name          = "state",
-		.revision      = 0,
-		.version       = XTABLES_VERSION,
-		.size          = XT_ALIGN(sizeof(struct xt_state_info)),
-		.userspacesize = XT_ALIGN(sizeof(struct xt_state_info)),
-		.help          = state_help,
-		.print         = state_print,
-		.save          = state_save,
-		.x6_parse      = state_parse,
-		.x6_options    = state_opts,
-	},
 };
 
 void _init(void)