Patchwork [libnftables,7/7] chain: handle attribute is relevant if only there is no name to use

login
register
mail settings
Submitter Tomasz Bursztyka
Date May 14, 2013, 10:51 a.m.
Message ID <1368528682-10041-8-git-send-email-tomasz.bursztyka@linux.intel.com>
Download mbox | patch
Permalink /patch/243674/
State Not Applicable
Delegated to: Pablo Neira
Headers show

Comments

Tomasz Bursztyka - May 14, 2013, 10:51 a.m.
While changing chain's settings, like its policy, it requires either the
handle or the name, but not both.

Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
 src/chain.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Pablo Neira - May 14, 2013, 10:20 p.m.
On Tue, May 14, 2013 at 01:51:22PM +0300, Tomasz Bursztyka wrote:
> While changing chain's settings, like its policy, it requires either the
> handle or the name, but not both.
> 
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
>  src/chain.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/chain.c b/src/chain.c
> index 1b1c3fe..e9a7896 100644
> --- a/src/chain.c
> +++ b/src/chain.c
> @@ -263,7 +263,8 @@ void nft_chain_nlmsg_build_payload(struct nlmsghdr *nlh, const struct nft_chain
>  		mnl_attr_put_u64(nlh, NFTA_COUNTER_BYTES, be64toh(c->bytes));
>  		mnl_attr_nest_end(nlh, nest);
>  	}
> -	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE))
> +	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE) &&
> +	    !(c->flags & (1 << NFT_CHAIN_ATTR_NAME)))
>  		mnl_attr_put_u64(nlh, NFTA_CHAIN_HANDLE, be64toh(c->handle));

The kernel will ignore the name if the handle is set. So no need to
make this artificial restriction in user-space.

>  	if (c->flags & (1 << NFT_CHAIN_ATTR_TYPE))
>  		mnl_attr_put_strz(nlh, NFTA_CHAIN_TYPE, c->type);
> -- 
> 1.8.2.1
> 
> --
> 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
--
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
Tomasz Bursztyka - May 15, 2013, 6:08 a.m.
Hi Pablo,
> On Tue, May 14, 2013 at 01:51:22PM +0300, Tomasz Bursztyka wrote:
>> While changing chain's settings, like its policy, it requires either the
>> handle or the name, but not both.
>>
>> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
>> ---
>>   src/chain.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/chain.c b/src/chain.c
>> index 1b1c3fe..e9a7896 100644
>> --- a/src/chain.c
>> +++ b/src/chain.c
>> @@ -263,7 +263,8 @@ void nft_chain_nlmsg_build_payload(struct nlmsghdr *nlh, const struct nft_chain
>>   		mnl_attr_put_u64(nlh, NFTA_COUNTER_BYTES, be64toh(c->bytes));
>>   		mnl_attr_nest_end(nlh, nest);
>>   	}
>> -	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE))
>> +	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE) &&
>> +	    !(c->flags & (1 << NFT_CHAIN_ATTR_NAME)))
>>   		mnl_attr_put_u64(nlh, NFTA_CHAIN_HANDLE, be64toh(c->handle));
> The kernel will ignore the name if the handle is set. So no need to
> make this artificial restriction in user-space.

No this not the case, have a look at net/netfilter/nf_tables_api.c in 
nf_tables_newchain(), lines 858-860:

if (nla[NFTA_CHAIN_HANDLE] && name &&
     !IS_ERR(nf_tables_chain_lookup(table, nla[NFTA_CHAIN_NAME])))
         return -EEXIST;

When handle and name are both present it means user wants to change the 
chain's name. (see line 882)
But in our case, when changing only the policy we don't touch the name, 
but libnftables provides it anyway thus failing on that test.||||

My patch is bogus anyway: I should add a marker that name has been 
changed first (and if only it was really different), and then handle it 
when building the message.


Tomasz
--
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 - May 15, 2013, 12:43 p.m.
On Wed, May 15, 2013 at 09:08:27AM +0300, Tomasz Bursztyka wrote:
[...]
> >The kernel will ignore the name if the handle is set. So no need to
> >make this artificial restriction in user-space.
> 
> No this not the case, have a look at net/netfilter/nf_tables_api.c
> in nf_tables_newchain(), lines 858-860:
> 
> if (nla[NFTA_CHAIN_HANDLE] && name &&
>     !IS_ERR(nf_tables_chain_lookup(table, nla[NFTA_CHAIN_NAME])))
>         return -EEXIST;
> 
> When handle and name are both present it means user wants to change
> the chain's name. (see line 882)
> But in our case, when changing only the policy we don't touch the
> name, but libnftables provides it anyway thus failing on that
> test.||||

But the handle number is built into the netlink message if the client
sets the NFT_CHAIN_ATTR_HANDLE. Looking at iptables-nftables, that
only happens in nft_chain_user_rename.

This seems to me like the client needs to be fixed not to set both
attributes at the same time (unless it wants a chain rename).

Where are you hitting this?

> My patch is bogus anyway: I should add a marker that name has been
> changed first (and if only it was really different), and then handle
> it when building the message.
--
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
Tomasz Bursztyka - May 15, 2013, 1:06 p.m.
Hi Pablo,
> But the handle number is built into the netlink message if the client
> sets the NFT_CHAIN_ATTR_HANDLE. Looking at iptables-nftables, that
> only happens in nft_chain_user_rename.
>
> This seems to me like the client needs to be fixed not to set both
> attributes at the same time (unless it wants a chain rename).
>
> Where are you hitting this?
>

I was actually playing on my own with libnftables.
It's easy: dump the chain list, then change the policy on one chain for 
instance, build the message to apply this change, send it...

We haven't hit the bug yet anywhere, because no code does such settings 
change after a dump, but we - or whatever app - surely will at some point.


Tomasz
--
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 - May 15, 2013, 1:40 p.m.
On Wed, May 15, 2013 at 04:06:26PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
> >But the handle number is built into the netlink message if the client
> >sets the NFT_CHAIN_ATTR_HANDLE. Looking at iptables-nftables, that
> >only happens in nft_chain_user_rename.
> >
> >This seems to me like the client needs to be fixed not to set both
> >attributes at the same time (unless it wants a chain rename).
> >
> >Where are you hitting this?
> >
> 
> I was actually playing on my own with libnftables.
> It's easy: dump the chain list, then change the policy on one chain
> for instance, build the message to apply this change, send it...
> 
> We haven't hit the bug yet anywhere, because no code does such
> settings change after a dump, but we - or whatever app - surely will
> at some point.

We should document that. I have a patch to add the doxygen doc to
libnftables. I prefer that user is in control of this, instead of
adding internal workarounds to avoid tricky situations.
--
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
Tomasz Bursztyka - May 15, 2013, 1:54 p.m.
Hi Pablo,
>> I was actually playing on my own with libnftables.
>> >It's easy: dump the chain list, then change the policy on one chain
>> >for instance, build the message to apply this change, send it...
>> >
>> >We haven't hit the bug yet anywhere, because no code does such
>> >settings change after a dump, but we - or whatever app - surely will
>> >at some point.
> We should document that. I have a patch to add the doxygen doc to
> libnftables. I prefer that user is in control of this, instead of
> adding internal workarounds to avoid tricky situations.

As you want, but it will complicate things for the user a bit.

Tomasz
--
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 - May 15, 2013, 2:28 p.m.
On Wed, May 15, 2013 at 04:54:46PM +0300, Tomasz Bursztyka wrote:
> Hi Pablo,
>
> >>>I was actually playing on my own with libnftables.
> >>>It's easy: dump the chain list, then change the policy on one chain
> >>>for instance, build the message to apply this change, send it...
> >>>
> >>>We haven't hit the bug yet anywhere, because no code does such
> >>>settings change after a dump, but we - or whatever app - surely will
> >>>at some point.
> >
> >We should document that. I have a patch to add the doxygen doc to
> >libnftables. I prefer that user is in control of this, instead of
> >adding internal workarounds to avoid tricky situations.
> 
> As you want, but it will complicate things for the user a bit.

Going back to the kernel code (line 882):

       if (nla[NFTA_CHAIN_HANDLE] && name)
                nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);

We can check in the chain name in the kernel and the name passed are
the same, in that case we skip nla_strlcpy.
--
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

Patch

diff --git a/src/chain.c b/src/chain.c
index 1b1c3fe..e9a7896 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -263,7 +263,8 @@  void nft_chain_nlmsg_build_payload(struct nlmsghdr *nlh, const struct nft_chain
 		mnl_attr_put_u64(nlh, NFTA_COUNTER_BYTES, be64toh(c->bytes));
 		mnl_attr_nest_end(nlh, nest);
 	}
-	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE))
+	if (c->flags & (1 << NFT_CHAIN_ATTR_HANDLE) &&
+	    !(c->flags & (1 << NFT_CHAIN_ATTR_NAME)))
 		mnl_attr_put_u64(nlh, NFTA_CHAIN_HANDLE, be64toh(c->handle));
 	if (c->flags & (1 << NFT_CHAIN_ATTR_TYPE))
 		mnl_attr_put_strz(nlh, NFTA_CHAIN_TYPE, c->type);