[nf-next PATCH v2 0/2] netfilter: xt_recent: Allow for larger hitcount values

[nf-next PATCH v2 0/2] netfilter: xt_recent: Allow for larger hitcount values

[Phil Sutter]

Changes since v1:
- Do not support insanely large hitcount values to limit the amount of
  storage allocated per entry
- Fix typo in subject of patch 1

Patch 2 lifts the restriction of 255 as max hitcount value by adjusting
XT_RECENT_MAX_NSTAMPS value and increasing required struct field sizes
accordingly.

In struct recent_entry, field 'nstamps' was 16bit in size of unclear
reasons. Patch 1 changes that to match field 'index' providing rationale
why it is sufficient, thus paving the way for keeping both at 16bit (and
avoiding a larger size for 'nstamps').

Phil Sutter (2):
  netfilter: xt_recent: Reduce size of struct recent_entry::nstamps
  netfilter: xt_recent: Lift restrictions on max hitcount value

 net/netfilter/xt_recent.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.43.0

[nf-next PATCH v2 1/2] netfilter: xt_recent: Reduce size of struct recent_entry::nstamps

[Phil Sutter]

There is no point in this change besides presenting its possibility
separate from a follow-up patch extending the size of both 'index' and
'nstamps' fields.

The value of 'nstamps' is initialized to 1 in recent_entry_init() and
adjusted in recent_entry_update() to match that of 'index' if it becomes
larger after being incremented. Since 'index' is of type u8, it will at
max become 255 (and wrap to 0 afterwards). Therefore, 'nstamps' will
also never exceed the value 255.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/xt_recent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index ef93e0d3bee0..60259280b2d5 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -70,7 +70,7 @@ struct recent_entry {
 	u_int16_t		family;
 	u_int8_t		ttl;
 	u_int8_t		index;
-	u_int16_t		nstamps;
+	u_int8_t		nstamps;
 	unsigned long		stamps[];
 };
 
-- 
2.43.0

Re: [nf-next PATCH v2 1/2] netfilter: xt_recent: Reduce size of struct recent_entry::nstamps

[Pablo Neira Ayuso]

Hi Phil,

On Fri, Jun 14, 2024 at 05:16:40PM +0200, Phil Sutter wrote:
> There is no point in this change besides presenting its possibility
> separate from a follow-up patch extending the size of both 'index' and
> 'nstamps' fields.
> 
> The value of 'nstamps' is initialized to 1 in recent_entry_init() and
> adjusted in recent_entry_update() to match that of 'index' if it becomes
> larger after being incremented. Since 'index' is of type u8, it will at
> max become 255 (and wrap to 0 afterwards). Therefore, 'nstamps' will
> also never exceed the value 255.

Series LGTM.

I'd suggest you collapse these two patches while keeping the
description above, because nstamps is shrinked here in 1/2 then it
gets back to original u16 in 2/2.

Maybe something like:

The value of 'nstamps' is initialized to 1 in recent_entry_init() and
adjusted in recent_entry_update() to match that of 'index' if it becomes
larger after being incremented. Since 'index' is of type u8, it will at
max become 255 (and wrap to 0 afterwards). Therefore, 'nstamps' will
also never exceed the value 255. But this patch expands 'index' to
u16 and then 'nstamps' needs to use u16 too, which exactly the
existing field size in the existing codebase.

I can do this mangling if you prefer to save you cycles, in such case
if you also like better wording, just let me know.

Thanks.

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/netfilter/xt_recent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
> index ef93e0d3bee0..60259280b2d5 100644
> --- a/net/netfilter/xt_recent.c
> +++ b/net/netfilter/xt_recent.c
> @@ -70,7 +70,7 @@ struct recent_entry {
>  	u_int16_t		family;
>  	u_int8_t		ttl;
>  	u_int8_t		index;
> -	u_int16_t		nstamps;
> +	u_int8_t		nstamps;
>  	unsigned long		stamps[];
>  };
>  
> -- 
> 2.43.0
> 
> 

Re: [nf-next PATCH v2 1/2] netfilter: xt_recent: Reduce size of struct recent_entry::nstamps

[Phil Sutter]

On Wed, Jun 26, 2024 at 06:12:43PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Fri, Jun 14, 2024 at 05:16:40PM +0200, Phil Sutter wrote:
> > There is no point in this change besides presenting its possibility
> > separate from a follow-up patch extending the size of both 'index' and
> > 'nstamps' fields.
> > 
> > The value of 'nstamps' is initialized to 1 in recent_entry_init() and
> > adjusted in recent_entry_update() to match that of 'index' if it becomes
> > larger after being incremented. Since 'index' is of type u8, it will at
> > max become 255 (and wrap to 0 afterwards). Therefore, 'nstamps' will
> > also never exceed the value 255.
> 
> Series LGTM.

Thanks for your review.

> I'd suggest you collapse these two patches while keeping the
> description above, because nstamps is shrinked here in 1/2 then it
> gets back to original u16 in 2/2.

ACK, that was the plan right from the start. :)

> Maybe something like:

I composed a new note to add to the second patch. Please review and let
me know if it's unclear or misleading.

Thanks, Phil

Re: [nf-next PATCH v2 1/2] netfilter: xt_recent: Reduce size of struct recent_entry::nstamps

[Pablo Neira Ayuso]

On Thu, Jun 27, 2024 at 12:40:58AM +0200, Phil Sutter wrote:
> On Wed, Jun 26, 2024 at 06:12:43PM +0200, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > On Fri, Jun 14, 2024 at 05:16:40PM +0200, Phil Sutter wrote:
> > > There is no point in this change besides presenting its possibility
> > > separate from a follow-up patch extending the size of both 'index' and
> > > 'nstamps' fields.
> > > 
> > > The value of 'nstamps' is initialized to 1 in recent_entry_init() and
> > > adjusted in recent_entry_update() to match that of 'index' if it becomes
> > > larger after being incremented. Since 'index' is of type u8, it will at
> > > max become 255 (and wrap to 0 afterwards). Therefore, 'nstamps' will
> > > also never exceed the value 255.
> > 
> > Series LGTM.
> 
> Thanks for your review.
> 
> > I'd suggest you collapse these two patches while keeping the
> > description above, because nstamps is shrinked here in 1/2 then it
> > gets back to original u16 in 2/2.
> 
> ACK, that was the plan right from the start. :)

Thanks, I have to admit splitting the patch in two helped me
understand a lot better when reviewing.

> > Maybe something like:
> 
> I composed a new note to add to the second patch. Please review and let
> me know if it's unclear or misleading.

LGTM, thanks

Re: [nf-next PATCH v2 1/2] netfilter: xt_recent: Reduce size of struct recent_entry::nstamps

[Phil Sutter]

On Thu, Jun 27, 2024 at 01:39:43AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 27, 2024 at 12:40:58AM +0200, Phil Sutter wrote:
> > On Wed, Jun 26, 2024 at 06:12:43PM +0200, Pablo Neira Ayuso wrote:
> > > Hi Phil,
> > > 
> > > On Fri, Jun 14, 2024 at 05:16:40PM +0200, Phil Sutter wrote:
> > > > There is no point in this change besides presenting its possibility
> > > > separate from a follow-up patch extending the size of both 'index' and
> > > > 'nstamps' fields.
> > > > 
> > > > The value of 'nstamps' is initialized to 1 in recent_entry_init() and
> > > > adjusted in recent_entry_update() to match that of 'index' if it becomes
> > > > larger after being incremented. Since 'index' is of type u8, it will at
> > > > max become 255 (and wrap to 0 afterwards). Therefore, 'nstamps' will
> > > > also never exceed the value 255.
> > > 
> > > Series LGTM.
> > 
> > Thanks for your review.
> > 
> > > I'd suggest you collapse these two patches while keeping the
> > > description above, because nstamps is shrinked here in 1/2 then it
> > > gets back to original u16 in 2/2.
> > 
> > ACK, that was the plan right from the start. :)
> 
> Thanks, I have to admit splitting the patch in two helped me
> understand a lot better when reviewing.

Cool! When restructuring patches for netfilter, I noticed a few times
how I suddenly was able to split things into surprisingly small sets
of changes which were much easier to explain in the commit message. So
we both benefit from this practice. :)

Cheers, Phil

[nf-next PATCH v2 2/2] netfilter: xt_recent: Lift restrictions on max hitcount value

[Phil Sutter]

Support tracking of up to 65535 packets per table entry instead of just
255 to better facilitate longer term tracking or higher throughput
scenarios.

Requested-by: Fabio <pedretti.fabio@gmail.com>
Link: https://bugzilla.netfilter.org/show_bug.cgi?id=1745
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/xt_recent.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index 60259280b2d5..588a5e6ad899 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -59,9 +59,9 @@ MODULE_PARM_DESC(ip_list_gid, "default owning group of /proc/net/xt_recent/* fil
 /* retained for backwards compatibility */
 static unsigned int ip_pkt_list_tot __read_mostly;
 module_param(ip_pkt_list_tot, uint, 0400);
-MODULE_PARM_DESC(ip_pkt_list_tot, "number of packets per IP address to remember (max. 255)");
+MODULE_PARM_DESC(ip_pkt_list_tot, "number of packets per IP address to remember (max. 65535)");
 
-#define XT_RECENT_MAX_NSTAMPS	256
+#define XT_RECENT_MAX_NSTAMPS	65536
 
 struct recent_entry {
 	struct list_head	list;
@@ -69,8 +69,8 @@ struct recent_entry {
 	union nf_inet_addr	addr;
 	u_int16_t		family;
 	u_int8_t		ttl;
-	u_int8_t		index;
-	u_int8_t		nstamps;
+	u_int16_t		index;
+	u_int16_t		nstamps;
 	unsigned long		stamps[];
 };
 
@@ -80,7 +80,7 @@ struct recent_table {
 	union nf_inet_addr	mask;
 	unsigned int		refcnt;
 	unsigned int		entries;
-	u8			nstamps_max_mask;
+	u_int16_t		nstamps_max_mask;
 	struct list_head	lru_list;
 	struct list_head	iphash[];
 };
-- 
2.43.0

Re: [nf-next PATCH v2 2/2] netfilter: xt_recent: Lift restrictions on max hitcount value

[Pablo Neira Ayuso]

On Fri, Jun 14, 2024 at 05:16:41PM +0200, Phil Sutter wrote:
> Support tracking of up to 65535 packets per table entry instead of just
> 255 to better facilitate longer term tracking or higher throughput
> scenarios.

Could you develop a bit more the use case to expand this? Do you have
an example rule for me?

> Requested-by: Fabio <pedretti.fabio@gmail.com>
> Link: https://bugzilla.netfilter.org/show_bug.cgi?id=1745

Hm, original bug report only refer to documentation update?

Is there a way to know what kernel support what value? I guess not,
only probing.

Thanks.

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  net/netfilter/xt_recent.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
> index 60259280b2d5..588a5e6ad899 100644
> --- a/net/netfilter/xt_recent.c
> +++ b/net/netfilter/xt_recent.c
> @@ -59,9 +59,9 @@ MODULE_PARM_DESC(ip_list_gid, "default owning group of /proc/net/xt_recent/* fil
>  /* retained for backwards compatibility */
>  static unsigned int ip_pkt_list_tot __read_mostly;
>  module_param(ip_pkt_list_tot, uint, 0400);
> -MODULE_PARM_DESC(ip_pkt_list_tot, "number of packets per IP address to remember (max. 255)");
> +MODULE_PARM_DESC(ip_pkt_list_tot, "number of packets per IP address to remember (max. 65535)");
>  
> -#define XT_RECENT_MAX_NSTAMPS	256
> +#define XT_RECENT_MAX_NSTAMPS	65536
>  
>  struct recent_entry {
>  	struct list_head	list;
> @@ -69,8 +69,8 @@ struct recent_entry {
>  	union nf_inet_addr	addr;
>  	u_int16_t		family;
>  	u_int8_t		ttl;
> -	u_int8_t		index;
> -	u_int8_t		nstamps;
> +	u_int16_t		index;
> +	u_int16_t		nstamps;
>  	unsigned long		stamps[];
>  };
>  
> @@ -80,7 +80,7 @@ struct recent_table {
>  	union nf_inet_addr	mask;
>  	unsigned int		refcnt;
>  	unsigned int		entries;
> -	u8			nstamps_max_mask;
> +	u_int16_t		nstamps_max_mask;
>  	struct list_head	lru_list;
>  	struct list_head	iphash[];
>  };
> -- 
> 2.43.0
> 
> 

Re: [nf-next PATCH v2 2/2] netfilter: xt_recent: Lift restrictions on max hitcount value

[Fabio Pedretti]

Il giorno ven 14 giu 2024 alle ore 17:24 Pablo Neira Ayuso
<pablo@netfilter.org> ha scritto:
>
> On Fri, Jun 14, 2024 at 05:16:41PM +0200, Phil Sutter wrote:
> > Support tracking of up to 65535 packets per table entry instead of just
> > 255 to better facilitate longer term tracking or higher throughput
> > scenarios.
>
> Could you develop a bit more the use case to expand this? Do you have
> an example rule for me?
>
> > Requested-by: Fabio <pedretti.fabio@gmail.com>
> > Link: https://bugzilla.netfilter.org/show_bug.cgi?id=1745
>
> Hm, original bug report only refer to documentation update?

I indeed opened the bug report mostly for the documentation, but also
wrote there:
"or, even better, make it possible to use a bigger value, since it is
useful to detect longer duration abuses"

I was trying to use the recent module to log IPs which generates lots
of new connections from the internal network, to detect misbehaving
clients (examples: misconfigured clients, torrent clients).
Given the recent limit of 255 I tried hashlimit, however I found the
recent module seems simpler and better to set up, perfect for the job,
also it has --set , --rcheck, --update and --reap options, to set
different trigger values to detect and keep IPs in the table.

Thanks.