[PATCH v2] netfilter: xt_hashlimit: fix build error caused by 64bit division

2017-09-07 Thread Vishwanath Pai 
64bit division causes build/link errors on 32bit architectures. It
prints out error messages like:

ERROR: "__aeabi_uldivmod" [net/netfilter/xt_hashlimit.ko] undefined!

The value of avg passed through by userspace in BYTE mode cannot exceed
U32_MAX. Which means 64bit division in user2rate_bytes is unnecessary.
To fix this I have changed the type of param 'user' to u32.

Since anything greater than U32_MAX is an invalid input we error out in
hashlimit_mt_check_common() when this is the case.

Changes in v2:
Making return type as u32 would cause an overflow for small
values of 'user' (for example 2, 3 etc). To avoid this I bumped up
'r' to u64 again as well as the return type. This is OK since the
variable that stores the result is u64. We still avoid 64bit
division here since 'user' is u32.

Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
Signed-off-by: Vishwanath Pai 
---
 net/netfilter/xt_hashlimit.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 10d4823..1c1941e 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Harald Welte ");
@@ -527,12 +528,12 @@ static u64 user2rate(u64 user)
}
 }
 
-static u64 user2rate_bytes(u64 user)
+static u64 user2rate_bytes(u32 user)
 {
u64 r;
 
-   r = user ? 0xULL / user : 0xULL;
-   r = (r - 1) << 4;
+   r = user ? U32_MAX / user : U32_MAX;
+   r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
return r;
 }
 
@@ -588,7 +589,8 @@ static void rateinfo_init(struct dsthash_ent *dh,
dh->rateinfo.prev_window = 0;
dh->rateinfo.current_rate = 0;
if (hinfo->cfg.mode & XT_HASHLIMIT_BYTES) {
-   dh->rateinfo.rate = user2rate_bytes(hinfo->cfg.avg);
+   dh->rateinfo.rate =
+   user2rate_bytes((u32)hinfo->cfg.avg);
if (hinfo->cfg.burst)
dh->rateinfo.burst =
hinfo->cfg.burst * dh->rateinfo.rate;
@@ -870,7 +872,7 @@ static int hashlimit_mt_check_common(const struct 
xt_mtchk_param *par,
 
/* Check for overflow. */
if (revision >= 3 && cfg->mode & XT_HASHLIMIT_RATE_MATCH) {
-   if (cfg->avg == 0) {
+   if (cfg->avg == 0 || cfg->avg > U32_MAX) {
pr_info("hashlimit invalid rate\n");
return -ERANGE;
}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] net:netfilter alloc xt_byteslimit_htable with wrong size

2017-09-07 Thread zhizhou . tian 
From: Zhizhou Tian 

struct xt_byteslimit_htable used hlist_head,
but alloc memory with sizeof(struct list_head)

Change-Id: I75bc60e47e0823700d4303c9d763b7995e3b7bb3
Signed-off-by: Zhizhou Tian 
---
 net/netfilter/xt_hashlimit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 10d4823..962ea4a 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -279,7 +279,7 @@ static int htable_create(struct net *net, struct 
hashlimit_cfg3 *cfg,
size = cfg->size;
} else {
size = (totalram_pages << PAGE_SHIFT) / 16384 /
-  sizeof(struct list_head);
+  sizeof(struct hlist_head);
if (totalram_pages > 1024 * 1024 * 1024 / PAGE_SIZE)
size = 8192;
if (size < 16)
@@ -287,7 +287,7 @@ static int htable_create(struct net *net, struct 
hashlimit_cfg3 *cfg,
}
/* FIXME: don't use vmalloc() here or anywhere else -HW */
hinfo = vmalloc(sizeof(struct xt_hashlimit_htable) +
-   sizeof(struct list_head) * size);
+   sizeof(struct hlist_head) * size);
if (hinfo == NULL)
return -ENOMEM;
*out_hinfo = hinfo;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] netfilter: xt_hashlimit: fix build error caused by 64bit division

2017-09-07 Thread Vishwanath Pai 
64bit division causes build/link errors on 32bit architectures. It
prints out error messages like:

ERROR: "__aeabi_uldivmod" [net/netfilter/xt_hashlimit.ko] undefined!

The value of avg passed through by userspace in BYTE mode cannot exceed
U32_MAX. Which means 64bit division in user2rate_bytes is unnecessary.

This fix changes the size of both the param as well as return type on
user2rate_bytes to u32.

Since anything greater than U32_MAX is an invalid input we error out in
hashlimit_mt_check_common() when this is the case.

Also fixed warning about const pointer conversion in cfg_copy().

Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
Signed-off-by: Vishwanath Pai 
---
 net/netfilter/xt_hashlimit.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 10d4823..1d818f1 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Harald Welte ");
@@ -527,12 +528,12 @@ static u64 user2rate(u64 user)
}
 }
 
-static u64 user2rate_bytes(u64 user)
+static u32 user2rate_bytes(u32 user)
 {
-   u64 r;
+   u32 r;
 
-   r = user ? 0xULL / user : 0xULL;
-   r = (r - 1) << 4;
+   r = user ? U32_MAX / user : U32_MAX;
+   r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT;
return r;
 }
 
@@ -588,7 +589,8 @@ static void rateinfo_init(struct dsthash_ent *dh,
dh->rateinfo.prev_window = 0;
dh->rateinfo.current_rate = 0;
if (hinfo->cfg.mode & XT_HASHLIMIT_BYTES) {
-   dh->rateinfo.rate = user2rate_bytes(hinfo->cfg.avg);
+   dh->rateinfo.rate =
+   user2rate_bytes((u32)hinfo->cfg.avg);
if (hinfo->cfg.burst)
dh->rateinfo.burst =
hinfo->cfg.burst * dh->rateinfo.rate;
@@ -870,7 +872,7 @@ static int hashlimit_mt_check_common(const struct 
xt_mtchk_param *par,
 
/* Check for overflow. */
if (revision >= 3 && cfg->mode & XT_HASHLIMIT_RATE_MATCH) {
-   if (cfg->avg == 0) {
+   if (cfg->avg == 0 || cfg->avg > U32_MAX) {
pr_info("hashlimit invalid rate\n");
return -ERANGE;
}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: WARN_ON() in __nf_hook_entries_try_shrink()

2017-09-07 Thread Stefano Brivio 
On Thu, 7 Sep 2017 17:01:12 -0700
Linus Torvalds  wrote:

> On shutdown I get this (edited down a bit to be more legible):
> 
>   Stopping firewalld - dynamic firewall daemon...
>   NETFILTER_CFG table=nat family=2 entries=55
>   NETFILTER_CFG table=mangle family=2 entries=40
>   NETFILTER_CFG table=raw family=2 entries=28
>   NETFILTER_CFG table=security family=2 entries=13
>   NETFILTER_CFG table=filter family=2 entries=93
>   [ cut here ]
>   WARNING: CPU: 1 PID: 21512 at net/netfilter/core.c:218
> __nf_hook_entries_try_shrink+0x106/0x130

Should be fixed by:

http://patchwork.ozlabs.org/patch/810570/

--
Stefano
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARN_ON() in __nf_hook_entries_try_shrink()

2017-09-07 Thread Linus Torvalds 
On shutdown I get this (edited down a bit to be more legible):

  Stopping firewalld - dynamic firewall daemon...
  NETFILTER_CFG table=nat family=2 entries=55
  NETFILTER_CFG table=mangle family=2 entries=40
  NETFILTER_CFG table=raw family=2 entries=28
  NETFILTER_CFG table=security family=2 entries=13
  NETFILTER_CFG table=filter family=2 entries=93
  [ cut here ]
  WARNING: CPU: 1 PID: 21512 at net/netfilter/core.c:218
__nf_hook_entries_try_shrink+0x106/0x130
  CPU: 1 PID: 21512 Comm: iptables-restor Not tainted
4.13.0-08555-gc0da4fa0d1a5 #7
  Hardware name: Dell Inc. XPS 13 9350/09JHRY, BIOS 1.4.17 05/10/2017
  RIP: 0010:__nf_hook_entries_try_shrink+0x106/0x130
  Call Trace:
 nf_unregister_net_hooks+0x117/0x240
 ipv4_hooks_unregister+0x60/0x70 [nf_conntrack_ipv4]
 nf_ct_netns_put+0x48/0x80 [nf_conntrack]
 conntrack_mt_destroy+0x15/0x20 [xt_conntrack]
 cleanup_match+0x43/0x70
 cleanup_entry+0x42/0xc0
 __do_replace+0x17a/0x1f0
 do_ipt_set_ctl+0x146/0x1b0
 nf_setsockopt+0x46/0x80
 ip_setsockopt+0x82/0xb0
 raw_setsockopt+0x34/0x40
 sock_common_setsockopt+0x14/0x20
 SyS_setsockopt+0x80/0xe0
 entry_SYSCALL_64_fastpath+0x13/0x94
[ .. warning repeats a few times .. ]
  ---[ end trace 56a6f5b20d97161d ]---
  NETFILTER_CFG table=broute family=7 entries=0
  NETFILTER_CFG table=nat family=7 entries=0
  NETFILTER_CFG table=filter family=7 entries=0
  NETFILTER_CFG table=mangle family=2 entries=6

and some searching notes that the kernel test robot already reported
this a few days ago but nobody reacted.

The kernel test robot seems to blame commit d3ad2c17b404 ("netfilter:
core: batch nf_unregister_net_hooks synchronize_net calls").

Hmm?

   Linus
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[libnftnl PATCH] chain: Don't print unset policy value in netlink debug

2017-09-07 Thread Phil Sutter 
The policy field was printed unconditionally, but if it wasn't set the
default value 0 was printed as 'policy drop' which is not correct.

Signed-off-by: Phil Sutter 
---
 src/chain.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/chain.c b/src/chain.c
index 29860c509a180..3190a775a0005 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -795,11 +795,19 @@ static int nftnl_chain_snprintf_default(char *buf, size_t 
size,
SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
if (c->flags & (1 << NFTNL_CHAIN_HOOKNUM)) {
-   ret = snprintf(buf+offset, len,
-  " type %s hook %s prio %d policy %s "
-  "packets %"PRIu64" bytes %"PRIu64"",
+   ret = snprintf(buf+offset, len, " type %s hook %s prio %d",
   c->type, nftnl_hooknum2str(c->family, 
c->hooknum),
-  c->prio, nftnl_verdict2str(c->policy),
+  c->prio);
+   SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+   if (c->flags & (1 << NFTNL_CHAIN_POLICY)) {
+   ret = snprintf(buf+offset, len, " policy %s",
+  nftnl_verdict2str(c->policy));
+   SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+   }
+
+   ret = snprintf(buf+offset, len,
+  " packets %"PRIu64" bytes %"PRIu64"",
   c->packets, c->bytes);
SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
-- 
2.13.1

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division

2017-09-07 Thread Geert Uytterhoeven 
Hi Arnd,

On Wed, Sep 6, 2017 at 9:57 PM, Arnd Bergmann  wrote:
> 64-bit division is expensive on 32-bit architectures, and
> requires a special function call to avoid a link error like:
>
> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>
> In the case of hashlimit_mt_common, we don't actually need a
> 64-bit operation, we can simply rewrite the function slightly
> to make that clear to the compiler.
>
> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
> Signed-off-by: Arnd Bergmann 

Thanks, this fixes a similar issue (__udivdi3 undefined) on m68k.

Acked-by: Geert Uytterhoeven 

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[ulogd2 PATCH] ulogd: use a RT scheduler by default

2017-09-07 Thread Arturo Borrero Gonzalez 
Is common that ulogd runs in scenarios where a lot of packets are to be logged.
If there are more packets than ulogd can handle, users can start seing log
messages like this:

 ulogd[556]: We are losing events. Please, consider using the clauses \
 `netlink_socket_buffer_size' and `netlink_socket_buffer_maxsize'

Which means that Netlink buffer overrun have happened.
There are several approaches to prevent this situation:

 * in the ruleset, limit the amount of packet queued for log
 * in the ruleset, instruct the kernel to use a queue-threshold
 * from userspace, increment Netlink buffer sizes
 * from userspace, configure ulogd to run as high priority process

The first 3 method can be configured by users at runtime.
This patch deals with the last method. SCHED_RR is configured by default,
with no associated configuration parameter for users, since I believe
this is common enough, and should produce no harm.

A similar approach is used in the conntrackd daemon.

Signed-off-by: Arturo Borrero Gonzalez 
---
 src/ulogd.c |   15 +++
 1 file changed, 15 insertions(+)

diff --git a/src/ulogd.c b/src/ulogd.c
index b85d0ee..68f 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -64,6 +64,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #ifdef DEBUG
@@ -1395,6 +1396,19 @@ static void signal_handler_task(int signal)
deliver_signal_pluginstances(signal);
 }
 
+static void set_scheduler(void)
+{
+   struct sched_param schedparam;
+   int sched_type;
+
+   schedparam.sched_priority = sched_get_priority_max(SCHED_RR);
+   sched_type = SCHED_RR;
+
+   if (sched_setscheduler(0, sched_type, ) < 0)
+   fprintf(stderr, "WARNING: scheduler configuration failed:"
+   " %s\n", strerror(errno));
+}
+
 static void print_usage(void)
 {
printf("ulogd Version %s\n", VERSION);
@@ -1589,6 +1603,7 @@ int main(int argc, char* argv[])
signal(SIGALRM, _handler);
signal(SIGUSR1, _handler);
signal(SIGUSR2, _handler);
+   set_scheduler();
 
ulogd_log(ULOGD_INFO, 
  "initialization finished, entering main loop\n");

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division

2017-09-07 Thread Arnd Bergmann 
On Thu, Sep 7, 2017 at 12:19 PM, Pablo Neira Ayuso  wrote:
> On Wed, Sep 06, 2017 at 10:48:22PM +0200, Arnd Bergmann wrote:
>> On Wed, Sep 6, 2017 at 10:22 PM, Vishwanath Pai  wrote:
>> > On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
>> >> 64-bit division is expensive on 32-bit architectures, and
>> >> requires a special function call to avoid a link error like:
>> >>
>> >> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
>> >> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
>> >>
>> >> In the case of hashlimit_mt_common, we don't actually need a
>> >> 64-bit operation, we can simply rewrite the function slightly
>> >> to make that clear to the compiler.
>> >>
>> >> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
>> >> Signed-off-by: Arnd Bergmann 
>> >> ---
>> >>  net/netfilter/xt_hashlimit.c | 5 -
>> >>  1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
>> >> index 10d48234f5f4..50b53d86eef5 100644
>> >> --- a/net/netfilter/xt_hashlimit.c
>> >> +++ b/net/netfilter/xt_hashlimit.c
>> >> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
>> >>  {
>> >>   u64 r;
>> >>
>> >> - r = user ? 0xULL / user : 0xULL;
>> >> + if (user > 0xULL)
>> >> + return 0;
>> >> +
>> >> + r = user ? 0xULL / (u32)user : 0xULL;
>> >>   r = (r - 1) << 4;
>> >>   return r;
>> >>  }
>> >>
>> >
>> > I have submitted another patch to fix this:
>> > https://patchwork.ozlabs.org/patch/809881/
>> >
>> > We have seen this problem before, I was careful not to introduce this
>> > again in the new patch but clearly I overlooked this particular line :(
>> >
>> > In the other cases we fixed it by replacing division with div64_u64().
>>
>> div64_u64() seems needlessly expensive here since the dividend
>> is known to be a 32-bit number. I guess the function is not called
>> frequently though, so it doesn't matter much.
>
> This is called from the packet path, only for the first packet for
> each new destination IP entry in the hashtable, still from the
> datapath. So if we can take something faster (for 32 bit arches) that
> is correct, I think it's sensible to take.
>
> Let me know in any case.

I think my version should be slightly better then, unless someone
finds something wrong with it.

   Arnd
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: xt_hashlimit: avoid 64-bit division

2017-09-07 Thread Pablo Neira Ayuso 
On Wed, Sep 06, 2017 at 10:48:22PM +0200, Arnd Bergmann wrote:
> On Wed, Sep 6, 2017 at 10:22 PM, Vishwanath Pai  wrote:
> > On 09/06/2017 03:57 PM, Arnd Bergmann wrote:
> >> 64-bit division is expensive on 32-bit architectures, and
> >> requires a special function call to avoid a link error like:
> >>
> >> net/netfilter/xt_hashlimit.o: In function `hashlimit_mt_common':
> >> xt_hashlimit.c:(.text+0x1328): undefined reference to `__aeabi_uldivmod'
> >>
> >> In the case of hashlimit_mt_common, we don't actually need a
> >> 64-bit operation, we can simply rewrite the function slightly
> >> to make that clear to the compiler.
> >>
> >> Fixes: bea74641e378 ("netfilter: xt_hashlimit: add rate match mode")
> >> Signed-off-by: Arnd Bergmann 
> >> ---
> >>  net/netfilter/xt_hashlimit.c | 5 -
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> >> index 10d48234f5f4..50b53d86eef5 100644
> >> --- a/net/netfilter/xt_hashlimit.c
> >> +++ b/net/netfilter/xt_hashlimit.c
> >> @@ -531,7 +531,10 @@ static u64 user2rate_bytes(u64 user)
> >>  {
> >>   u64 r;
> >>
> >> - r = user ? 0xULL / user : 0xULL;
> >> + if (user > 0xULL)
> >> + return 0;
> >> +
> >> + r = user ? 0xULL / (u32)user : 0xULL;
> >>   r = (r - 1) << 4;
> >>   return r;
> >>  }
> >>
> >
> > I have submitted another patch to fix this:
> > https://patchwork.ozlabs.org/patch/809881/
> >
> > We have seen this problem before, I was careful not to introduce this
> > again in the new patch but clearly I overlooked this particular line :(
> >
> > In the other cases we fixed it by replacing division with div64_u64().
> 
> div64_u64() seems needlessly expensive here since the dividend
> is known to be a 32-bit number. I guess the function is not called
> frequently though, so it doesn't matter much.

This is called from the packet path, only for the first packet for
each new destination IP entry in the hashtable, still from the
datapath. So if we can take something faster (for 32 bit arches) that
is correct, I think it's sensible to take.

Let me know in any case.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html