* [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing @ 2014-02-12 9:27 Florian Westphal 2014-02-12 9:27 ` [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL Florian Westphal 2014-02-13 10:10 ` [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing Jozsef Kadlecsik 0 siblings, 2 replies; 8+ messages in thread From: Florian Westphal @ 2014-02-12 9:27 UTC (permalink / raw) To: netfilter-devel; +Cc: Florian Westphal hash:net,iface supports matching on the bridge port as well, but userspace currently doesn't handle it correctly as it passes in 'physdev:eth0' instead of 'eth0'+IPSET_OPT_PHYSDEV. Signed-off-by: Florian Westphal <fw@strlen.de> --- lib/parse.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/parse.c b/lib/parse.c index f1c1f0e..4db872e 100644 --- a/lib/parse.c +++ b/lib/parse.c @@ -1753,14 +1753,15 @@ ipset_parse_iface(struct ipset_session *session, { struct ipset_data *data; int offset = 0, err = 0; + static const char pdev_prefix[]="physdev:"; assert(session); assert(opt == IPSET_OPT_IFACE); assert(str); data = ipset_session_data(session); - if (STREQ(str, "physdev:")) { - offset = 8; + if (STRNEQ(str, pdev_prefix, strlen(pdev_prefix))) { + offset = strlen(pdev_prefix); err = ipset_data_set(data, IPSET_OPT_PHYSDEV, str); if (err < 0) return err; -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL 2014-02-12 9:27 [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing Florian Westphal @ 2014-02-12 9:27 ` Florian Westphal 2014-02-13 10:14 ` Jozsef Kadlecsik 2014-02-13 10:10 ` [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing Jozsef Kadlecsik 1 sibling, 1 reply; 8+ messages in thread From: Florian Westphal @ 2014-02-12 9:27 UTC (permalink / raw) To: netfilter-devel; +Cc: Florian Westphal ipset_data_get returns NULL if the attribute is not available, causes when running 'ipset list': $ ipset -N foo hash:ip $ ipset list Segmentation fault (core dumped) Signed-off-by: Florian Westphal <fw@strlen.de> --- Not sure about this patch, the missing attr is IPSET_OPT_MEMSIZE. Is the kernel supposed to send it along (i am on latest nf tree)? diff --git a/lib/print.c b/lib/print.c index f81c074..9ad4bab 100644 --- a/lib/print.c +++ b/lib/print.c @@ -378,6 +378,8 @@ ipset_print_number(char *buf, unsigned int len, assert(data); number = ipset_data_get(data, opt); + if (!number) + return 0; maxsize = ipset_data_sizeof(opt, AF_INET); D("opt: %u, maxsize %zu", opt, maxsize); if (maxsize == sizeof(uint8_t)) -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL 2014-02-12 9:27 ` [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL Florian Westphal @ 2014-02-13 10:14 ` Jozsef Kadlecsik 2014-02-13 11:17 ` Florian Westphal 0 siblings, 1 reply; 8+ messages in thread From: Jozsef Kadlecsik @ 2014-02-13 10:14 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel Hi Florian, On Wed, 12 Feb 2014, Florian Westphal wrote: > ipset_data_get returns NULL if the attribute is not available, causes when > running 'ipset list': > > $ ipset -N foo hash:ip > $ ipset list > Segmentation fault (core dumped) I'm unable to reproduce it. Please give me more information on the environment: architecture, kernel, ipset userspace versions, compiler version. > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > Not sure about this patch, the missing attr is IPSET_OPT_MEMSIZE. > Is the kernel supposed to send it along (i am on latest nf tree)? How do you know which attr is missing? IPSET_OPT_MEMSIZE should always be sent by the kernel, look at mtype_head in ip_set_hash_gen.h. > > diff --git a/lib/print.c b/lib/print.c > index f81c074..9ad4bab 100644 > --- a/lib/print.c > +++ b/lib/print.c > @@ -378,6 +378,8 @@ ipset_print_number(char *buf, unsigned int len, > assert(data); > > number = ipset_data_get(data, opt); > + if (!number) > + return 0; > maxsize = ipset_data_sizeof(opt, AF_INET); > D("opt: %u, maxsize %zu", opt, maxsize); > if (maxsize == sizeof(uint8_t)) > -- > 1.8.1.5 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL 2014-02-13 10:14 ` Jozsef Kadlecsik @ 2014-02-13 11:17 ` Florian Westphal 2014-02-13 11:31 ` Jozsef Kadlecsik 0 siblings, 1 reply; 8+ messages in thread From: Florian Westphal @ 2014-02-13 11:17 UTC (permalink / raw) To: Jozsef Kadlecsik; +Cc: Florian Westphal, netfilter-devel Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > On Wed, 12 Feb 2014, Florian Westphal wrote: > > > ipset_data_get returns NULL if the attribute is not available, causes when > > running 'ipset list': > > > > $ ipset -N foo hash:ip > > $ ipset list > > Segmentation fault (core dumped) > > I'm unable to reproduce it. Please give me more information on the > environment: architecture, kernel, ipset userspace versions, compiler > version. latest ipset git version, net-next tree from this morning. > > Not sure about this patch, the missing attr is IPSET_OPT_MEMSIZE. > > Is the kernel supposed to send it along (i am on latest nf tree)? > > How do you know which attr is missing? IPSET_OPT_MEMSIZE should always be > sent by the kernel, look at mtype_head in ip_set_hash_gen.h. You are right. The attribute is there. I bisected this down to commit commit 2dfb973c0dcc6d22113e2370f461f1733035baaf Author: Vytas Dauksa <vytas.dauksa@smoothwall.net> Date: Tue Dec 17 14:01:44 2013 +0000 add markmask for hash:ip,mark data type The problem is that this commit breaks userspace abi. Minimum fix that makes latest ipset userspace work again is this, most likely this needs to be corrected on kernel side as well (afaics its not yet in net-next or nf trees): diff --git a/include/libipset/linux_ip_set.h b/include/libipset/linux_ip_set.h index c2bae85..d9beec5 100644 --- a/include/libipset/linux_ip_set.h +++ b/include/libipset/linux_ip_set.h @@ -90,7 +90,6 @@ enum { IPSET_ATTR_GC, IPSET_ATTR_HASHSIZE, IPSET_ATTR_MAXELEM, - IPSET_ATTR_MARKMASK, IPSET_ATTR_NETMASK, IPSET_ATTR_PROBES, IPSET_ATTR_RESIZE, @@ -99,6 +98,7 @@ enum { IPSET_ATTR_ELEMENTS, IPSET_ATTR_REFERENCES, IPSET_ATTR_MEMSIZE, + IPSET_ATTR_MARKMASK, __IPSET_ATTR_CREATE_MAX, }; @@ -140,7 +140,6 @@ enum ipset_errno { IPSET_ERR_EXIST, IPSET_ERR_INVALID_CIDR, IPSET_ERR_INVALID_NETMASK, - IPSET_ERR_INVALID_MARKMASK, IPSET_ERR_INVALID_FAMILY, IPSET_ERR_TIMEOUT, IPSET_ERR_REFERENCED, @@ -148,6 +147,7 @@ enum ipset_errno { IPSET_ERR_IPADDR_IPV6, IPSET_ERR_COUNTER, IPSET_ERR_COMMENT, + IPSET_ERR_INVALID_MARKMASK, /* Type specific error codes */ IPSET_ERR_TYPE_SPECIFIC = 4352, ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL 2014-02-13 11:17 ` Florian Westphal @ 2014-02-13 11:31 ` Jozsef Kadlecsik 0 siblings, 0 replies; 8+ messages in thread From: Jozsef Kadlecsik @ 2014-02-13 11:31 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Thu, 13 Feb 2014, Florian Westphal wrote: > Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > On Wed, 12 Feb 2014, Florian Westphal wrote: > > > > > ipset_data_get returns NULL if the attribute is not available, causes when > > > running 'ipset list': > > > > > > $ ipset -N foo hash:ip > > > $ ipset list > > > Segmentation fault (core dumped) > > > > I'm unable to reproduce it. Please give me more information on the > > environment: architecture, kernel, ipset userspace versions, compiler > > version. > > latest ipset git version, net-next tree from this morning. > > > > Not sure about this patch, the missing attr is IPSET_OPT_MEMSIZE. > > > Is the kernel supposed to send it along (i am on latest nf tree)? > > > > How do you know which attr is missing? IPSET_OPT_MEMSIZE should always be > > sent by the kernel, look at mtype_head in ip_set_hash_gen.h. > > You are right. The attribute is there. I bisected this down to commit > > commit 2dfb973c0dcc6d22113e2370f461f1733035baaf > Author: Vytas Dauksa <vytas.dauksa@smoothwall.net> > Date: Tue Dec 17 14:01:44 2013 +0000 > > add markmask for hash:ip,mark data type > > The problem is that this commit breaks userspace abi. Minimum fix > that makes latest ipset userspace work again is this, most likely this > needs to be corrected on kernel side as well (afaics its not yet in > net-next or nf trees): Good catch indeed! The attribute then should be moved just after IPSET_ATTR_MARK in include/uapi/linux/netfilter/ipset/ipset.h and run "make update_includes" so that include/libipset/linux_ip_set.h gets refreshed. Could you prepare a patch? Best regards, Jozsef > diff --git a/include/libipset/linux_ip_set.h b/include/libipset/linux_ip_set.h > index c2bae85..d9beec5 100644 > --- a/include/libipset/linux_ip_set.h > +++ b/include/libipset/linux_ip_set.h > @@ -90,7 +90,6 @@ enum { > IPSET_ATTR_GC, > IPSET_ATTR_HASHSIZE, > IPSET_ATTR_MAXELEM, > - IPSET_ATTR_MARKMASK, > IPSET_ATTR_NETMASK, > IPSET_ATTR_PROBES, > IPSET_ATTR_RESIZE, > @@ -99,6 +98,7 @@ enum { > IPSET_ATTR_ELEMENTS, > IPSET_ATTR_REFERENCES, > IPSET_ATTR_MEMSIZE, > + IPSET_ATTR_MARKMASK, > > __IPSET_ATTR_CREATE_MAX, > }; > @@ -140,7 +140,6 @@ enum ipset_errno { > IPSET_ERR_EXIST, > IPSET_ERR_INVALID_CIDR, > IPSET_ERR_INVALID_NETMASK, > - IPSET_ERR_INVALID_MARKMASK, > IPSET_ERR_INVALID_FAMILY, > IPSET_ERR_TIMEOUT, > IPSET_ERR_REFERENCED, > @@ -148,6 +147,7 @@ enum ipset_errno { > IPSET_ERR_IPADDR_IPV6, > IPSET_ERR_COUNTER, > IPSET_ERR_COMMENT, > + IPSET_ERR_INVALID_MARKMASK, > > /* Type specific error codes */ > IPSET_ERR_TYPE_SPECIFIC = 4352, > -- > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing 2014-02-12 9:27 [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing Florian Westphal 2014-02-12 9:27 ` [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL Florian Westphal @ 2014-02-13 10:10 ` Jozsef Kadlecsik 2014-02-13 11:01 ` Florian Westphal 1 sibling, 1 reply; 8+ messages in thread From: Jozsef Kadlecsik @ 2014-02-13 10:10 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel Hi Florian, On Wed, 12 Feb 2014, Florian Westphal wrote: > hash:net,iface supports matching on the bridge port as well, > but userspace currently doesn't handle it correctly as it passes > in 'physdev:eth0' instead of 'eth0'+IPSET_OPT_PHYSDEV. I think the userspace does handle the case: looking at your patch, it's exactly the same as the original one. It is nicer, so I'm happy to apply it, but the description - as far as I see - doesn't fit. Maybe your kernel is not compiled with BRIDGE_NETFILTER enabled? That's required for the functionality at the kernel side. > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > lib/parse.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/parse.c b/lib/parse.c > index f1c1f0e..4db872e 100644 > --- a/lib/parse.c > +++ b/lib/parse.c > @@ -1753,14 +1753,15 @@ ipset_parse_iface(struct ipset_session *session, > { > struct ipset_data *data; > int offset = 0, err = 0; > + static const char pdev_prefix[]="physdev:"; > > assert(session); > assert(opt == IPSET_OPT_IFACE); > assert(str); > > data = ipset_session_data(session); > - if (STREQ(str, "physdev:")) { > - offset = 8; > + if (STRNEQ(str, pdev_prefix, strlen(pdev_prefix))) { > + offset = strlen(pdev_prefix); > err = ipset_data_set(data, IPSET_OPT_PHYSDEV, str); > if (err < 0) > return err; > -- > 1.8.1.5 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing 2014-02-13 10:10 ` [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing Jozsef Kadlecsik @ 2014-02-13 11:01 ` Florian Westphal 2014-02-13 11:22 ` Jozsef Kadlecsik 0 siblings, 1 reply; 8+ messages in thread From: Florian Westphal @ 2014-02-13 11:01 UTC (permalink / raw) To: Jozsef Kadlecsik; +Cc: Florian Westphal, netfilter-devel Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > Hi Florian, > > On Wed, 12 Feb 2014, Florian Westphal wrote: > > > hash:net,iface supports matching on the bridge port as well, > > but userspace currently doesn't handle it correctly as it passes > > in 'physdev:eth0' instead of 'eth0'+IPSET_OPT_PHYSDEV. > > I think the userspace does handle the case: looking at your patch, it's > exactly the same as the original one. It is nicer, so I'm happy to apply > it, but the description - as far as I see - doesn't fit. It will expand to if (strcmp("physdev:eth0", "physdev:") == 0) which is not true. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing 2014-02-13 11:01 ` Florian Westphal @ 2014-02-13 11:22 ` Jozsef Kadlecsik 0 siblings, 0 replies; 8+ messages in thread From: Jozsef Kadlecsik @ 2014-02-13 11:22 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Thu, 13 Feb 2014, Florian Westphal wrote: > Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > > > On Wed, 12 Feb 2014, Florian Westphal wrote: > > > > > hash:net,iface supports matching on the bridge port as well, > > > but userspace currently doesn't handle it correctly as it passes > > > in 'physdev:eth0' instead of 'eth0'+IPSET_OPT_PHYSDEV. > > > > I think the userspace does handle the case: looking at your patch, it's > > exactly the same as the original one. It is nicer, so I'm happy to apply > > it, but the description - as far as I see - doesn't fit. > > It will expand to > > if (strcmp("physdev:eth0", "physdev:") == 0) > > which is not true. Ohh, right. And then the whole string is passed to the kernel and therefore the "physdev:" part is kept at listing. Patch is applied, thanks! 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-13 11:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-12 9:27 [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing Florian Westphal 2014-02-12 9:27 ` [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL Florian Westphal 2014-02-13 10:14 ` Jozsef Kadlecsik 2014-02-13 11:17 ` Florian Westphal 2014-02-13 11:31 ` Jozsef Kadlecsik 2014-02-13 10:10 ` [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing Jozsef Kadlecsik 2014-02-13 11:01 ` Florian Westphal 2014-02-13 11:22 ` Jozsef Kadlecsik
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).