| Message ID | 1452614825-19749-1-git-send-email-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
Hi Florian, On Tue, 12 Jan 2016, Florian Westphal wrote: > Jozsef says: > The correct behaviour is that if we have > ipset create test1 hash:net,iface > ipset add test1 0.0.0.0/0,eth0 > iptables -A INPUT -m set --match-set test1 src,src > > then the rule should match for any traffic coming in through eth0. > > This removes the -EINVAL runtime test to make matching work > in case packet arrived via the specified interface. No, the patch actually would break the set type. In order to support /0 prefixes, cidr + 1 is stored internally. Zero value means "empty slot/bucket". > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1297092 From the bugreport we neither know the kernel version nor the exact iptables command. It might be that the rule is added to a chain from where the input interface is not available. Best regards, Jozsef > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > diff --git a/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c > index 43d8c98..f0f688d 100644 > --- a/net/netfilter/ipset/ip_set_hash_netiface.c > +++ b/net/netfilter/ipset/ip_set_hash_netiface.c > @@ -164,8 +164,6 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb, > }; > struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set); > > - if (e.cidr == 0) > - return -EINVAL; > if (adt == IPSET_TEST) > e.cidr = HOST_MASK; > > @@ -377,8 +375,6 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb, > }; > struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set); > > - if (e.cidr == 0) > - return -EINVAL; > if (adt == IPSET_TEST) > e.cidr = HOST_MASK; > > -- > 2.4.10 > > -- > 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 > - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- 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
Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > Hi Florian, > > On Tue, 12 Jan 2016, Florian Westphal wrote: > > > Jozsef says: > > The correct behaviour is that if we have > > ipset create test1 hash:net,iface > > ipset add test1 0.0.0.0/0,eth0 > > iptables -A INPUT -m set --match-set test1 src,src > > > > then the rule should match for any traffic coming in through eth0. > > > > This removes the -EINVAL runtime test to make matching work > > in case packet arrived via the specified interface. > > No, the patch actually would break the set type. In order to support /0 > prefixes, cidr + 1 is stored internally. Zero value means "empty > slot/bucket". Hmm, but matching is broken currently, the rule quoted above never matches. And its exaclty because of if (e.cidr == 0) is true. Tested with nf-next tree. Before patch: never matches After patch: matches for all packets from eth0 -- 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 Tue, 12 Jan 2016, Florian Westphal wrote: > Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > > > On Tue, 12 Jan 2016, Florian Westphal wrote: > > > > > Jozsef says: > > > The correct behaviour is that if we have > > > ipset create test1 hash:net,iface > > > ipset add test1 0.0.0.0/0,eth0 > > > iptables -A INPUT -m set --match-set test1 src,src > > > > > > then the rule should match for any traffic coming in through eth0. > > > > > > This removes the -EINVAL runtime test to make matching work > > > in case packet arrived via the specified interface. > > > > No, the patch actually would break the set type. In order to support /0 > > prefixes, cidr + 1 is stored internally. Zero value means "empty > > slot/bucket". > > Hmm, but matching is broken currently, the rule quoted above never > matches. And its exaclty because of if (e.cidr == 0) is true. > > Tested with nf-next tree. > Before patch: never matches > After patch: matches for all packets from eth0 I'll look into it, then something just resurfaces this way. Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- 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 Tue, 12 Jan 2016, Florian Westphal wrote: > Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > > > On Tue, 12 Jan 2016, Florian Westphal wrote: > > > > > Jozsef says: > > > The correct behaviour is that if we have > > > ipset create test1 hash:net,iface > > > ipset add test1 0.0.0.0/0,eth0 > > > iptables -A INPUT -m set --match-set test1 src,src > > > > > > then the rule should match for any traffic coming in through eth0. > > > > > > This removes the -EINVAL runtime test to make matching work > > > in case packet arrived via the specified interface. > > > > No, the patch actually would break the set type. In order to support /0 > > prefixes, cidr + 1 is stored internally. Zero value means "empty > > slot/bucket". > > Hmm, but matching is broken currently, the rule quoted above never > matches. And its exaclty because of if (e.cidr == 0) is true. > > Tested with nf-next tree. > Before patch: never matches > After patch: matches for all packets from eth0 I was wrong and the patch is correct, the removed condition is just unnecessary and breaks to match /0. I have already applied the patch in the ipset tree and added a new test to verify the case. Thanks, Florian! Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- 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, Please apply the patch to the nf tree, thanks! On Tue, 12 Jan 2016, Florian Westphal wrote: > Jozsef says: > The correct behaviour is that if we have > ipset create test1 hash:net,iface > ipset add test1 0.0.0.0/0,eth0 > iptables -A INPUT -m set --match-set test1 src,src > > then the rule should match for any traffic coming in through eth0. > > This removes the -EINVAL runtime test to make matching work > in case packet arrived via the specified interface. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1297092 > Signed-off-by: Florian Westphal <fw@strlen.de> Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> Best regards, Jozsef > --- > diff --git a/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c > index 43d8c98..f0f688d 100644 > --- a/net/netfilter/ipset/ip_set_hash_netiface.c > +++ b/net/netfilter/ipset/ip_set_hash_netiface.c > @@ -164,8 +164,6 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb, > }; > struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set); > > - if (e.cidr == 0) > - return -EINVAL; > if (adt == IPSET_TEST) > e.cidr = HOST_MASK; > > @@ -377,8 +375,6 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb, > }; > struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set); > > - if (e.cidr == 0) > - return -EINVAL; > if (adt == IPSET_TEST) > e.cidr = HOST_MASK; > > -- > 2.4.10 > > -- > 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 > - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- 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, Jan 13, 2016 at 09:35:02AM +0100, Jozsef Kadlecsik wrote: > Hi Pablo, > > Please apply the patch to the nf tree, thanks! > > On Tue, 12 Jan 2016, Florian Westphal wrote: > > > Jozsef says: > > The correct behaviour is that if we have > > ipset create test1 hash:net,iface > > ipset add test1 0.0.0.0/0,eth0 > > iptables -A INPUT -m set --match-set test1 src,src > > > > then the rule should match for any traffic coming in through eth0. > > > > This removes the -EINVAL runtime test to make matching work > > in case packet arrived via the specified interface. > > > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1297092 > > Signed-off-by: Florian Westphal <fw@strlen.de> > > Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> Applied, thanks! This applies cleanly to 4.3 and 4.4, so will be requesting -stable for these two. -- 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/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c index 43d8c98..f0f688d 100644 --- a/net/netfilter/ipset/ip_set_hash_netiface.c +++ b/net/netfilter/ipset/ip_set_hash_netiface.c @@ -164,8 +164,6 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb, }; struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set); - if (e.cidr == 0) - return -EINVAL; if (adt == IPSET_TEST) e.cidr = HOST_MASK; @@ -377,8 +375,6 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb, }; struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set); - if (e.cidr == 0) - return -EINVAL; if (adt == IPSET_TEST) e.cidr = HOST_MASK;
Jozsef says: The correct behaviour is that if we have ipset create test1 hash:net,iface ipset add test1 0.0.0.0/0,eth0 iptables -A INPUT -m set --match-set test1 src,src then the rule should match for any traffic coming in through eth0. This removes the -EINVAL runtime test to make matching work in case packet arrived via the specified interface. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1297092 Signed-off-by: Florian Westphal <fw@strlen.de> ---