Re: [PATCH nf-next v2 1/2] netfilter: Fix potential null pointer dereference

[Liping Zhang]

2016-09-28 11:08 GMT+08:00 Liping Zhang :
> Hi Feng,
>
> 2016-09-28 9:23 GMT+08:00 Feng Gao :
>> Hi Aaraon,
>>
>> On Tue, Sep 27, 2016 at 9:38 PM, Aaron Conole  wrote:
>>> It's possible for nf_hook_entry_head to return NULL if two
>>> nf_unregister_net_hook calls happen simultaneously with a single hook
>>
>> The critical region of nf_unregister_net_hook is protected by _hook_mutex.
>> When it would be called simultaneously?
>
> This is unrelated to race condition.
>
> Suppose that only the last nf_hook_entry exist, and two callers want to do
> un-register work.
>
> The first one will remove it successfully, after the end of the work, the
> second one will enter the critical section, but it will see the NULL pointer.
> Because the last nf_hook_entry was already removed by the first one.
>
>>
>> Regards
>> Feng
>>
>>> entry in the list.  This fix ensures that no null pointer dereference
>>> could occur when such a race happens.
>>>
>>> Signed-off-by: Aaron Conole 

I read the commit log again, I think the description here is a
little confusing indeed.
--
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

[ANNOUNCE] Netdev 1.2 updates (27th September, 2016)

[Hajime Tazaki]

Hello folks,

The Linux netdev 1.2 conference will start on next week and
I hope everyone is ready to join us.

== program ==

The full list of accepted sessions (keynotes, workshops,
talks, bofs) are available at the following page.

http://netdevconf.org/1.2/accepted-sessions.html

We will have a great series of the talks;  thank you for
your contributions for the great conference.

We're heavily working on finalizing the program;  we will
announce it very soon on the web page.  Stay tuned.

== travel information ==

You might be the first time to visit Tokyo/Japan (or even
Asian countries).  Several trip tips (transportation, venue
access, etc)  are listed in our web page; please check it
out before your trip.

http://netdevconf.org/1.2/travel.html


== mailing list ==

We have received a couple of requests to prepare one-time
mailing list only for the netdev 1.2 attendees.  We will
prepare the list asap; please freely use the list to
chat/discuss/communicate among the conference attendees.

== sponsors ==

We have finally a great list of our sponsors.  Without such
a huge support, the conference never makes it happen.  Many
many thanks !

- Platinum
Verizon, Facebook, Cumulus Networks
- Gold
Mojatatu Networks, VMWare, Google, NTT, LinkedIn, Cisco
- Silver
NetApp, IIJ, Netronome, SolarFlare, Mellanox, Sophos,
RedHat, Intel (new)
- Bronze
Zen Load Balancer
- Media sponsor
LWN.net



We're really looking forward to seeing you in Tokyo.  We
hope you will have a safe and great trip.

-- Hajime

--
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 v2] netfilter: xt_hashlimit: Fix link error in 32bit arch because of 64bit division

[Vishwanath Pai]

v2:
Remove unnecessary div64_u64 around constants

--

Fix link error in 32bit arch because of 64bit division

Division of 64bit integers will cause linker error undefined reference
to `__udivdi3'. Fix this by replacing divisions with div64_64

Signed-off-by: Vishwanath Pai 

---
 net/netfilter/xt_hashlimit.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 44a095e..200a9d8 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -467,17 +467,18 @@ static u64 user2credits(u64 user, int revision)
/* If multiplying would overflow... */
if (user > 0x / (HZ*CREDITS_PER_JIFFY_v1))
/* Divide first. */
-   return (user / XT_HASHLIMIT_SCALE) *\
+   return div64_u64(user, XT_HASHLIMIT_SCALE) *\
HZ * CREDITS_PER_JIFFY_v1;
 
-   return (user * HZ * CREDITS_PER_JIFFY_v1) \
-   / XT_HASHLIMIT_SCALE;
+   return div64_u64((user * HZ * CREDITS_PER_JIFFY_v1),
+ XT_HASHLIMIT_SCALE);
} else {
if (user > 0x / (HZ*CREDITS_PER_JIFFY))
-   return (user / XT_HASHLIMIT_SCALE_v2) *\
+   return div64_u64(user, XT_HASHLIMIT_SCALE_v2) *\
HZ * CREDITS_PER_JIFFY;
 
-   return (user * HZ * CREDITS_PER_JIFFY) / XT_HASHLIMIT_SCALE_v2;
+   return div64_u64((user * HZ * CREDITS_PER_JIFFY),
+XT_HASHLIMIT_SCALE_v2);
}
 }
 
-- 
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 nf-next v2 0/2] fixes for recent nf_compact hooks

[Aaron Conole]

Two possible error conditions were caught during an extended testing
session, and by a build robot.  These patches fix the two issues (a
missing handler when config is changed, and a potential NULL
dereference).

Aaron Conole (2):
  netfilter: Fix potential null pointer dereference
  nf_set_hooks_head: acommodate different kconfig

 net/netfilter/core.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.5.5

--
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 nf-next v2 1/2] netfilter: Fix potential null pointer dereference

[Aaron Conole]

It's possible for nf_hook_entry_head to return NULL if two
nf_unregister_net_hook calls happen simultaneously with a single hook
entry in the list.  This fix ensures that no null pointer dereference
could occur when such a race happens.

Signed-off-by: Aaron Conole 
---
 net/netfilter/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index 360c63d..e58e420 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct 
nf_hook_ops *reg)
 
mutex_lock(_hook_mutex);
hooks_entry = nf_hook_entry_head(net, reg);
-   if (hooks_entry->orig_ops == reg) {
+   if (hooks_entry && hooks_entry->orig_ops == reg) {
nf_set_hooks_head(net, reg,
  nf_entry_dereference(hooks_entry->next));
goto unlock;
-- 
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

Re: [PATCH] Fix link error in 32bit arch because of 64bit division

[Eric Dumazet]

On Tue, 2016-09-27 at 03:42 -0400, Vishwanath Pai wrote:
> Fix link error in 32bit arch because of 64bit division
> 
> Division of 64bit integers will cause linker error undefined reference
> to `__udivdi3'. Fix this by replacing divisions with div64_64
> 
> Signed-off-by: Vishwanath Pai 
> 
> ---
>  net/netfilter/xt_hashlimit.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
> index 44a095e..7fc694e 100644
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -465,19 +465,20 @@ static u64 user2credits(u64 user, int revision)
>  {
>   if (revision == 1) {
>   /* If multiplying would overflow... */
> - if (user > 0x / (HZ*CREDITS_PER_JIFFY_v1))
> + if (user > div64_u64(0x, (HZ*CREDITS_PER_JIFFY_v1)))

How can this be needed ? 0x is 32bits, compiler knows how to
compute 0x / (HZ*CREDITS_PER_JIFFY_v1) itself, without using a
64 bit divide !

Please be selective.

>   /* Divide first. */
> - return (user / XT_HASHLIMIT_SCALE) *\
> + return div64_u64(user, XT_HASHLIMIT_SCALE) *\
>   HZ * CREDITS_PER_JIFFY_v1;
>  
> - return (user * HZ * CREDITS_PER_JIFFY_v1) \
> - / XT_HASHLIMIT_SCALE;
> + return div64_u64((user * HZ * CREDITS_PER_JIFFY_v1),
> +   XT_HASHLIMIT_SCALE);
>   } else {
> - if (user > 0x / (HZ*CREDITS_PER_JIFFY))
> - return (user / XT_HASHLIMIT_SCALE_v2) *\

Probably same remark here.

> + if (user > div64_u64(0x, 
> (HZ*CREDITS_PER_JIFFY)))
> + return div64_u64(user, XT_HASHLIMIT_SCALE_v2) *\
>   HZ * CREDITS_PER_JIFFY;
>  
> - return (user * HZ * CREDITS_PER_JIFFY) / XT_HASHLIMIT_SCALE_v2;
> + return div64_u64((user * HZ * CREDITS_PER_JIFFY),
> +  XT_HASHLIMIT_SCALE_v2);
>   }
>  }
>  


--
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] Fix link error in 32bit arch because of 64bit division

[Liping Zhang]

Hi  Vishwanath Pai,

2016-09-27 15:42 GMT+08:00 Vishwanath Pai :
> Fix link error in 32bit arch because of 64bit division

This should be "netfilter: xt_hashlimit: fix ... "

>
> --- a/net/netfilter/xt_hashlimit.c
> +++ b/net/netfilter/xt_hashlimit.c
> @@ -465,19 +465,20 @@ static u64 user2credits(u64 user, int revision)
>  {
> if (revision == 1) {
> /* If multiplying would overflow... */
> -   if (user > 0x / (HZ*CREDITS_PER_JIFFY_v1))
> +   if (user > div64_u64(0x, (HZ*CREDITS_PER_JIFFY_v1)))

Here divisor and dividend are all 32-bit integer, so covert "/" to div64_u64
seems unnecessary.

> /* Divide first. */
> -   return (user / XT_HASHLIMIT_SCALE) *\
> +   return div64_u64(user, XT_HASHLIMIT_SCALE) *\
> HZ * CREDITS_PER_JIFFY_v1;
>
> -   return (user * HZ * CREDITS_PER_JIFFY_v1) \
> -   / XT_HASHLIMIT_SCALE;
> +   return div64_u64((user * HZ * CREDITS_PER_JIFFY_v1),
> + XT_HASHLIMIT_SCALE);
> } else {
> -   if (user > 0x / (HZ*CREDITS_PER_JIFFY))
> -   return (user / XT_HASHLIMIT_SCALE_v2) *\
> +   if (user > div64_u64(0x, 
> (HZ*CREDITS_PER_JIFFY)))

0x and "HZ*CREDITS_PER_JIFFY" are both
constant, and GCC will do constant folding optimization, so I think
convert "/" to div64_u64 here is also unnecessary.

> +   return div64_u64(user, XT_HASHLIMIT_SCALE_v2) *\
> HZ * CREDITS_PER_JIFFY;
>
> -   return (user * HZ * CREDITS_PER_JIFFY) / 
> XT_HASHLIMIT_SCALE_v2;
> +   return div64_u64((user * HZ * CREDITS_PER_JIFFY),
> +XT_HASHLIMIT_SCALE_v2);
> }
>  }
>
--
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] Fix link error in 32bit arch because of 64bit division

[Vishwanath Pai]

Fix link error in 32bit arch because of 64bit division

Division of 64bit integers will cause linker error undefined reference
to `__udivdi3'. Fix this by replacing divisions with div64_64

Signed-off-by: Vishwanath Pai 

---
 net/netfilter/xt_hashlimit.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 44a095e..7fc694e 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -465,19 +465,20 @@ static u64 user2credits(u64 user, int revision)
 {
if (revision == 1) {
/* If multiplying would overflow... */
-   if (user > 0x / (HZ*CREDITS_PER_JIFFY_v1))
+   if (user > div64_u64(0x, (HZ*CREDITS_PER_JIFFY_v1)))
/* Divide first. */
-   return (user / XT_HASHLIMIT_SCALE) *\
+   return div64_u64(user, XT_HASHLIMIT_SCALE) *\
HZ * CREDITS_PER_JIFFY_v1;
 
-   return (user * HZ * CREDITS_PER_JIFFY_v1) \
-   / XT_HASHLIMIT_SCALE;
+   return div64_u64((user * HZ * CREDITS_PER_JIFFY_v1),
+ XT_HASHLIMIT_SCALE);
} else {
-   if (user > 0x / (HZ*CREDITS_PER_JIFFY))
-   return (user / XT_HASHLIMIT_SCALE_v2) *\
+   if (user > div64_u64(0x, 
(HZ*CREDITS_PER_JIFFY)))
+   return div64_u64(user, XT_HASHLIMIT_SCALE_v2) *\
HZ * CREDITS_PER_JIFFY;
 
-   return (user * HZ * CREDITS_PER_JIFFY) / XT_HASHLIMIT_SCALE_v2;
+   return div64_u64((user * HZ * CREDITS_PER_JIFFY),
+XT_HASHLIMIT_SCALE_v2);
}
 }
 
-- 
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: [PATCH nf-next] netfilter: xt_osf: Use explicit member assignment to avoid implicit no padding rule

[Liping Zhang]

Hi Feng,

2016-09-27 14:00 GMT+08:00 Gao Feng :
> Hi Liping,
>
>>
>> This xt_osf_user_finger{} is carefully designed, no padding now, and
>> will not be changed in the future, otherwise backward compatibility will
>> be broken.
>
> Yes, there is no padding now. So it is ok to use memcmp now.
> I am afraid the struct would be modified for other requirements.

This is structure was passed by netlink attribute, so modify it will
break backward compatibility.

>
> If it is never changed forever, it is ok certainly.
>
>>
>> I don't think this convert is necessary, actually it is a little ugly, and 
>> will
>> increase the maintenance burden.
>
> I just want the codes don't depend any implicit rule.
>
> It is a tradeoff. If never change, needn't do any convert.
> If may change, the memcmp is a little dangerous.
>
> Regards
> Feng
--
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 nf-next] netfilter: xt_osf: Use explicit member assignment to avoid implicit no padding rule

[Gao Feng]

Hi Liping,


On Tue, Sep 27, 2016 at 1:49 PM, Liping Zhang  wrote:
> Hi Feng,
>
> 2016-09-27 12:39 GMT+08:00  :
>> From: Gao Feng 
>>
>> Current xt_osf codes use memcmp to check if two user fingers are same,
>> so it depends on that the struct xt_osf_user_finger is no padding.
>> It is one implicit rule, and is not good to maintain.
>>
>> Now use zero memory and assign the members explicitly.
>>
>> Signed-off-by: Gao Feng 
>> ---
>>  net/netfilter/xt_osf.c | 32 ++--
>>  1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/netfilter/xt_osf.c b/net/netfilter/xt_osf.c
>> index 2455b69..9793670 100644
>> --- a/net/netfilter/xt_osf.c
>> +++ b/net/netfilter/xt_osf.c
>> @@ -61,6 +61,34 @@ static const struct nla_policy xt_osf_policy[OSF_ATTR_MAX 
>> + 1] = {
>> [OSF_ATTR_FINGER]   = { .len = sizeof(struct xt_osf_user_finger) 
>> },
>>  };
>>
>> +static void copy_user_finger(struct xt_osf_user_finger *dst,
>> +const struct xt_osf_user_finger *src)
>> +{
>> +#define OSF_COPY_MEMBER(mem)   dst->mem = src->mem
>> +
>> +   int i;
>> +
>> +   OSF_COPY_MEMBER(wss.wc);
>> +   OSF_COPY_MEMBER(wss.val);
>> +
>> +   OSF_COPY_MEMBER(ttl);
>> +   OSF_COPY_MEMBER(df);
>> +   OSF_COPY_MEMBER(ss);
>> +   OSF_COPY_MEMBER(mss);
>> +   OSF_COPY_MEMBER(opt_num);
>> +
>> +   memcpy(dst->genre, src->genre, sizeof(dst->genre));
>> +   memcpy(dst->version, src->version, sizeof(dst->version));
>> +   memcpy(dst->subtype, src->subtype, sizeof(dst->subtype));
>> +
>> +   for (i = 0; i < MAX_IPOPTLEN; ++i) {
>> +   OSF_COPY_MEMBER(opt[i].kind);
>> +   OSF_COPY_MEMBER(opt[i].length);
>> +   OSF_COPY_MEMBER(opt[i].wc.wc);
>> +   OSF_COPY_MEMBER(opt[i].wc.val);
>> +   }
>> +}
>> +
>
> This xt_osf_user_finger{} is carefully designed, no padding now, and
> will not be changed in the future, otherwise backward compatibility will
> be broken.

Yes, there is no padding now. So it is ok to use memcmp now.
I am afraid the struct would be modified for other requirements.

If it is never changed forever, it is ok certainly.

>
> I don't think this convert is necessary, actually it is a little ugly, and 
> will
> increase the maintenance burden.

I just want the codes don't depend any implicit rule.

It is a tradeoff. If never change, needn't do any convert.
If may change, the memcmp is a little dangerous.

Regards
Feng


--
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