[PATCH 0/1] ipset fix for nf/net

[PATCH 0/1] ipset fix for nf/net

[Jozsef Kadlecsik]

Hi Pablo,

Since the last batch of ipset related patches there have been a bugreport
and the next patch fixes it. Please consider applying it.

Best regards,
Jozsef

The following changes since commit c2d421e171868586939c328dfb91bab840fe4c49:
  Florian Westphal (1):
        netfilter: nf_nat: fix race when unloading protocol modules

are available in the git repository at:

  git://blackhole.kfki.hu/nf master

Jozsef Kadlecsik (1):
      netfilter: ipset: bitmap:ip,mac: fix listing with timeout

 net/netfilter/ipset/ip_set_bitmap_ipmac.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

[PATCH 1/1] netfilter: ipset: bitmap:ip,mac: fix listing with timeout

[Jozsef Kadlecsik]

The type when timeout support was enabled, could not list all elements,
just the first ones which could fit into one netlink message: it just
did not continue listing after the first message.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/ipset/ip_set_bitmap_ipmac.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
index 0f92dc2..d7df6ac 100644
--- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
+++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
@@ -339,7 +339,11 @@ bitmap_ipmac_tlist(const struct ip_set *set,
 nla_put_failure:
 	nla_nest_cancel(skb, nested);
 	ipset_nest_end(skb, atd);
-	return -EMSGSIZE;
+	if (unlikely(id == first)) {
+		cb->args[2] = 0;
+		return -EMSGSIZE;
+	}
+	return 0;
 }
 
 static int
-- 
1.7.0.4

Re: [PATCH 1/1] netfilter: ipset: bitmap:ip,mac: fix listing with timeout

[Pablo Neira Ayuso]

Hi Jozsef,

On Sat, Apr 13, 2013 at 02:51:14PM +0200, Jozsef Kadlecsik wrote:
> The type when timeout support was enabled, could not list all elements,
> just the first ones which could fit into one netlink message: it just
> did not continue listing after the first message.
>
> Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> ---
>  net/netfilter/ipset/ip_set_bitmap_ipmac.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> index 0f92dc2..d7df6ac 100644
> --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> @@ -339,7 +339,11 @@ bitmap_ipmac_tlist(const struct ip_set *set,
>  nla_put_failure:
>  	nla_nest_cancel(skb, nested);
>  	ipset_nest_end(skb, atd);

I think this ipset_nest_end should be after the id == first checking.
It doesn't make sense for the -EMSGSIZE case.

BTW, in the first message, where `first' is unset, id will never equal
first and you will always return success even if you could not add one
single nested attribute into the message.

> -	return -EMSGSIZE;
> +	if (unlikely(id == first)) {
> +		cb->args[2] = 0;
> +		return -EMSGSIZE;
> +	}
> +	return 0;

Regards.

Re: [PATCH 1/1] netfilter: ipset: bitmap:ip,mac: fix listing with timeout

[Jozsef Kadlecsik]

Hi Pablo,

On Tue, 16 Apr 2013, Pablo Neira Ayuso wrote:

> On Sat, Apr 13, 2013 at 02:51:14PM +0200, Jozsef Kadlecsik wrote:
> > The type when timeout support was enabled, could not list all elements,
> > just the first ones which could fit into one netlink message: it just
> > did not continue listing after the first message.
> >
> > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > ---
> >  net/netfilter/ipset/ip_set_bitmap_ipmac.c |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > index 0f92dc2..d7df6ac 100644
> > --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > @@ -339,7 +339,11 @@ bitmap_ipmac_tlist(const struct ip_set *set,
> >  nla_put_failure:
> >  	nla_nest_cancel(skb, nested);
> >  	ipset_nest_end(skb, atd);
> 
> I think this ipset_nest_end should be after the id == first checking.
> It doesn't make sense for the -EMSGSIZE case.

Yes, that could be moved there - candidate for the nf-next tree?
 
> BTW, in the first message, where `first' is unset, id will never equal
> first and you will always return success even if you could not add one
> single nested attribute into the message.

"first" is always initialized: it's either zero (the id of the first 
entry) or the id of the next one where listing must be continued.
 
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 1/1] netfilter: ipset: bitmap:ip,mac: fix listing with timeout

[Pablo Neira Ayuso]

Hi Jozsef,

On Tue, Apr 16, 2013 at 09:16:18PM +0200, Jozsef Kadlecsik wrote:
> Hi Pablo,
> 
> On Tue, 16 Apr 2013, Pablo Neira Ayuso wrote:
> 
> > On Sat, Apr 13, 2013 at 02:51:14PM +0200, Jozsef Kadlecsik wrote:
> > > The type when timeout support was enabled, could not list all elements,
> > > just the first ones which could fit into one netlink message: it just
> > > did not continue listing after the first message.
> > >
> > > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > > ---
> > >  net/netfilter/ipset/ip_set_bitmap_ipmac.c |    6 +++++-
> > >  1 files changed, 5 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > > index 0f92dc2..d7df6ac 100644
> > > --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > > +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > > @@ -339,7 +339,11 @@ bitmap_ipmac_tlist(const struct ip_set *set,
> > >  nla_put_failure:
> > >  	nla_nest_cancel(skb, nested);
> > >  	ipset_nest_end(skb, atd);
> > 
> > I think this ipset_nest_end should be after the id == first checking.
> > It doesn't make sense for the -EMSGSIZE case.
> 
> Yes, that could be moved there - candidate for the nf-next tree?

Yes. This is not critical, send me a follow up this in a follow up
patch for nf-next.

> > BTW, in the first message, where `first' is unset, id will never equal
> > first and you will always return success even if you could not add one
> > single nested attribute into the message.
> 
> "first" is always initialized: it's either zero (the id of the first 
> entry) or the id of the next one where listing must be continued.

I see, that's OK.

I have applied this patch. Thanks.