[PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing

[PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing

[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

[PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL

[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

Re: [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing

[Jozsef Kadlecsik]

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

Re: [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL

[Jozsef Kadlecsik]

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

Re: [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing

[Florian Westphal]

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.

Re: [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL

[Florian Westphal]

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,

Re: [PATCH ipset 1/2] lib: fix ifname 'physdev:' prefix parsing

[Jozsef Kadlecsik]

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

Re: [PATCH ipset 2/2] lib: don't segfault when ipset_data_get returns NULL

[Jozsef Kadlecsik]

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