Re: [PATCH] xt_recent: Fix false hit_count match

[Patrick McHardy <kaber@trash.net>]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Thomas Jarosch wrote:
> On Friday, 19. March 2010 16:41:49 you wrote:
>   
>>> Maybe this is related to the xt_recent
>>> proc interface creating the entry
>>> (with a zero hit count)?
>>>       
>> Mhh, looking at that patch again, I think it should actually do:
>>
>> if (!info->hit_count || ++hits >= info->hit_count)
>>     ...
>>
>> since a hit_count of 0 implies that the user just wants to check for the
>> presence of the entry. Thomas, could you give that a try?
>>     
>
> The new code works. Isn't that almost the same as reverting
> the original patch? info->hit_count == 0 will match again.
>
> So we could just go back to
>
> "if (++hits >= info->hit_count)"
>
> Or am I missing something?
>
> Clearly your new version is more readable about the intent.

Thomas, before I send this upstream with a Tested-by tag in your name,
could you please confirm that this is the change you've actually tested?

Thanks.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1462 bytes --]

commit ef1691504c83ba3eb636c0cfd3ed33f7a6d0b4ee
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Mar 22 18:25:20 2010 +0100

    netfilter: xt_recent: fix regression in rules using a zero hit_count
    
    Commit 8ccb92ad (netfilter: xt_recent: fix false match) fixed supposedly
    false matches in rules using a zero hit_count. As it turns out there is
    nothing false about these matches and people are actually using entries
    with a hit_count of zero to make rules dependant on addresses inserted
    manually through /proc.
    
    Since this slipped past the eyes of three reviewers, instead of
    reverting the commit in question, this patch explicitly checks
    for a hit_count of zero to make the intentions more clear.
    
    Reported-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
    Tested-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
    Cc: stable@kernel.org
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index 7073dbb..971d172 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -267,7 +267,7 @@ recent_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 		for (i = 0; i < e->nstamps; i++) {
 			if (info->seconds && time_after(time, e->stamps[i]))
 				continue;
-			if (info->hit_count && ++hits >= info->hit_count) {
+			if (!info->hit_count || ++hits >= info->hit_count) {
 				ret = !ret;
 				break;
 			}