| Message ID | 1368528682-10041-8-git-send-email-tomasz.bursztyka@linux.intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
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
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
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
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
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
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
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
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);
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(-)