[PATCH ulogd2 0/3] Three small bugfixes

[PATCH ulogd2 0/3] Three small bugfixes

[corubba]

This patchset contains three small bugfixes for the NFCT, GPRINT and
IPFIX plugins of ulogd2.

The first patch adds a missing newline to a log message of the NFCT
input plugin. The seconds patch adds the missing comma separator after
an ip address in the output of the GRPINT output plugin. The third patch
fixes the timer in the IPFIX output plugin, which (I assume) is meant to
ensure the messages are sent out in a timely manner.

The first two patches are pretty trivial and self-explanatory. The third
patch is based on my interpretation of how the plugin is supposed to
work, because this architectural detail isn't explained in the original
patchset nor discussed in their mailinglist-threads [0] [1]. I was also
unable to locate the original 2009 code from Holger Eitzenberger that
this is based on for comparison. I may have drawn the wrong conclusion,
so please feel free to form your own opinion if it is correct.


[0] https://lore.kernel.org/netfilter-devel/523542b5-d629-54d9-2a90-468a9cb3aba7@juaristi.eus/
[1] https://lore.kernel.org/netfilter-devel/20190426075807.7528-1-a@juaristi.eus/


Corubba Smith (3):
  nfct: add newline to reliable log message
  gprint: fix comma after ip addresses
  ipfix: re-arm send timer

 input/flow/ulogd_inpflow_NFCT.c   | 2 +-
 output/ipfix/ulogd_output_IPFIX.c | 8 ++++++--
 output/ulogd_output_GPRINT.c      | 6 ++++++
 3 files changed, 13 insertions(+), 3 deletions(-)

--
2.48.1

[PATCH ulogd2 1/3] nfct: add newline to reliable log message

[corubba]

Missing since 4bc3b22e.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 input/flow/ulogd_inpflow_NFCT.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/input/flow/ulogd_inpflow_NFCT.c b/input/flow/ulogd_inpflow_NFCT.c
index 899b7e3..983c8d6 100644
--- a/input/flow/ulogd_inpflow_NFCT.c
+++ b/input/flow/ulogd_inpflow_NFCT.c
@@ -1327,7 +1327,7 @@ static int constructor_nfct_events(struct ulogd_pluginstance *upi)
 		setsockopt(nfct_fd(cpi->cth), SOL_NETLINK,
 				NETLINK_NO_ENOBUFS, &on, sizeof(int));
 		ulogd_log(ULOGD_NOTICE, "NFCT reliable logging "
-					"has been enabled.");
+					"has been enabled.\n");
 	}
 	cpi->nfct_fd.fd = nfct_fd(cpi->cth);
 	cpi->nfct_fd.cb = &read_cb_nfct;
--
2.48.1

[PATCH ulogd2 2/3] gprint: fix comma after ip addresses

[corubba]

Gone missing in f04bf679.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 output/ulogd_output_GPRINT.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/output/ulogd_output_GPRINT.c b/output/ulogd_output_GPRINT.c
index 37829fa..d95ca9d 100644
--- a/output/ulogd_output_GPRINT.c
+++ b/output/ulogd_output_GPRINT.c
@@ -179,9 +179,15 @@ static int gprint_interp(struct ulogd_pluginstance *upi)
 			if (!inet_ntop(family, addr, buf + size, rem))
 				break;
 			ret = strlen(buf + size);
+			rem -= ret;
+			size += ret;

+			ret = snprintf(buf+size, rem, ",");
+			if (ret < 0)
+				break;
 			rem -= ret;
 			size += ret;
+
 			break;
 		}
 		default:
--
2.48.1

Re: [PATCH ulogd2 2/3] gprint: fix comma after ip addresses

[Pablo Neira Ayuso]

On Sat, Feb 08, 2025 at 02:49:49PM +0100, corubba wrote:
> Gone missing in f04bf679.

ulogd2$ git show f04bf679
fatal: ambiguous argument 'f04bf679': unknown revision or path not in the working tree.

???

> Signed-off-by: Corubba Smith <corubba@gmx.de>
> ---
>  output/ulogd_output_GPRINT.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/output/ulogd_output_GPRINT.c b/output/ulogd_output_GPRINT.c
> index 37829fa..d95ca9d 100644
> --- a/output/ulogd_output_GPRINT.c
> +++ b/output/ulogd_output_GPRINT.c
> @@ -179,9 +179,15 @@ static int gprint_interp(struct ulogd_pluginstance *upi)
>  			if (!inet_ntop(family, addr, buf + size, rem))
>  				break;
>  			ret = strlen(buf + size);
> +			rem -= ret;
> +			size += ret;
> 
> +			ret = snprintf(buf+size, rem, ",");
> +			if (ret < 0)
> +				break;
>  			rem -= ret;
>  			size += ret;
> +
>  			break;
>  		}
>  		default:
> --
> 2.48.1
> 
> 

Re: [PATCH ulogd2 2/3] gprint: fix comma after ip addresses

[corubba]

On 08.02.25 22:20, Pablo Neira Ayuso wrote:
> On Sat, Feb 08, 2025 at 02:49:49PM +0100, corubba wrote:
>> Gone missing in f04bf679.
>
> ulogd2$ git show f04bf679
> fatal: ambiguous argument 'f04bf679': unknown revision or path not in the working tree.
>
> ???

The full commit id is f04bf6794d1153abd2c3b0bfededd9403d79acf6, which
does exist [0]. And that `git show` command works in my local clone of
the repo, showing the commit in question.

[0] https://git.netfilter.org/ulogd2/commit/?id=f04bf679

>
>> Signed-off-by: Corubba Smith <corubba@gmx.de>
>> ---
>>  output/ulogd_output_GPRINT.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/output/ulogd_output_GPRINT.c b/output/ulogd_output_GPRINT.c
>> index 37829fa..d95ca9d 100644
>> --- a/output/ulogd_output_GPRINT.c
>> +++ b/output/ulogd_output_GPRINT.c
>> @@ -179,9 +179,15 @@ static int gprint_interp(struct ulogd_pluginstance *upi)
>>  			if (!inet_ntop(family, addr, buf + size, rem))
>>  				break;
>>  			ret = strlen(buf + size);
>> +			rem -= ret;
>> +			size += ret;
>>
>> +			ret = snprintf(buf+size, rem, ",");
>> +			if (ret < 0)
>> +				break;
>>  			rem -= ret;
>>  			size += ret;
>> +
>>  			break;
>>  		}
>>  		default:
>> --
>> 2.48.1
>>
>>
>

Re: [PATCH ulogd2 2/3] gprint: fix comma after ip addresses

[Pablo Neira Ayuso]

On Sat, Feb 08, 2025 at 10:30:04PM +0100, corubba wrote:
> On 08.02.25 22:20, Pablo Neira Ayuso wrote:
> > On Sat, Feb 08, 2025 at 02:49:49PM +0100, corubba wrote:
> >> Gone missing in f04bf679.
> >
> > ulogd2$ git show f04bf679
> > fatal: ambiguous argument 'f04bf679': unknown revision or path not in the working tree.
> >
> > ???
> 
> The full commit id is f04bf6794d1153abd2c3b0bfededd9403d79acf6, which
> does exist [0]. And that `git show` command works in my local clone of
> the repo, showing the commit in question.
> 
> [0] https://git.netfilter.org/ulogd2/commit/?id=f04bf679

My fault, I was quickly checking on a stale working copy of ulogd2, apologies.

Re: [PATCH ulogd2 2/3] gprint: fix comma after ip addresses

[Jeremy Sowden]

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

On 2025-02-08, at 22:20:32 +0100, Pablo Neira Ayuso wrote:
> On Sat, Feb 08, 2025 at 02:49:49PM +0100, corubba wrote:
> > Gone missing in f04bf679.
> 
> ulogd2$ git show f04bf679
> fatal: ambiguous argument 'f04bf679': unknown revision or path not in the working tree.
> 
> ???

f04bf6794d11 ("gprint, oprint: use inet_ntop to format ip addresses").
It includes this change:

-                       ret = snprintf(buf+size, rem, "%u.%u.%u.%u,",
-                               NIPQUAD(key->u.value.ui32));
-                       if (ret < 0)
+                       ipv4addr.s_addr = key->u.value.ui32;
+                       if (!inet_ntop(AF_INET, &ipv4addr, buf + size, rem))
                                break;
+                       ret = strlen(buf + size);
+

The snprintf appends a comma after the IP address.

> > Signed-off-by: Corubba Smith <corubba@gmx.de>
> > ---
> >  output/ulogd_output_GPRINT.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/output/ulogd_output_GPRINT.c b/output/ulogd_output_GPRINT.c
> > index 37829fa..d95ca9d 100644
> > --- a/output/ulogd_output_GPRINT.c
> > +++ b/output/ulogd_output_GPRINT.c
> > @@ -179,9 +179,15 @@ static int gprint_interp(struct ulogd_pluginstance *upi)
> >  			if (!inet_ntop(family, addr, buf + size, rem))
> >  				break;
> >  			ret = strlen(buf + size);
> > +			rem -= ret;
> > +			size += ret;
> > 
> > +			ret = snprintf(buf+size, rem, ",");
> > +			if (ret < 0)
> > +				break;
> >  			rem -= ret;
> >  			size += ret;
> > +
> >  			break;
> >  		}
> >  		default:
> > --
> > 2.48.1
> > 
> > 
> 

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 931 bytes --]

Re: [PATCH ulogd2 2/3] gprint: fix comma after ip addresses

[Jeremy Sowden]

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

On 2025-02-08, at 14:49:49 +0100, corubba wrote:
> Gone missing in f04bf679.
> 
> Signed-off-by: Corubba Smith <corubba@gmx.de>
> ---
>  output/ulogd_output_GPRINT.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/output/ulogd_output_GPRINT.c b/output/ulogd_output_GPRINT.c
> index 37829fa..d95ca9d 100644
> --- a/output/ulogd_output_GPRINT.c
> +++ b/output/ulogd_output_GPRINT.c
> @@ -179,9 +179,15 @@ static int gprint_interp(struct ulogd_pluginstance *upi)
>  			if (!inet_ntop(family, addr, buf + size, rem))
>  				break;
>  			ret = strlen(buf + size);
> +			rem -= ret;
> +			size += ret;
> 
> +			ret = snprintf(buf+size, rem, ",");
> +			if (ret < 0)
> +				break;
>  			rem -= ret;
>  			size += ret;
> +
>  			break;
>  		}
>  		default:

My suggestion would be to follow ulgod_output_OPRINT.c (completely
untested):

diff --git a/output/ulogd_output_GPRINT.c b/output/ulogd_output_GPRINT.c
index 37829fa49e9d..b18fe3a0838c 100644
--- a/output/ulogd_output_GPRINT.c
+++ b/output/ulogd_output_GPRINT.c
@@ -155,6 +155,7 @@ static int gprint_interp(struct ulogd_pluginstance *upi)
                        size += ret;
                        break;
                case ULOGD_RET_IPADDR: {
+                       char addrbuf[INET6_ADDRSTRLEN + 1] = "";
                        struct in6_addr ipv6addr;
                        struct in_addr ipv4addr;
                        int family;
@@ -176,10 +177,11 @@ static int gprint_interp(struct ulogd_pluginstance *upi)
                                addr = &ipv4addr;
                                family = AF_INET;
                        }
-                       if (!inet_ntop(family, addr, buf + size, rem))
+                       if (!inet_ntop(family, addr, addrbuf, sizeof(addrbuf)))
+                               break;
+                       ret = snprintf(buf + size, rem, "%s,", addrbuf);
+                       if (ret < 0)
                                break;
-                       ret = strlen(buf + size);
-
                        rem -= ret;
                        size += ret;
                        break;

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 931 bytes --]

[PATCH ulogd2 3/3] ipfix: re-arm send timer

[corubba]

I am not sure what this timer was meant to do. My best guess is to send
an ipfix message every second if there is data, as to make sure reports
go out in a timely manner. Otherwise a message is only sent when adding
another flow would go past the max mtu, which may take a while if there
isn't much (filtered) traffic.

Timers in ulogd only fire once; if you want them to fire repeatedly
(which I guess was the intention here), you need to re-arm them in the
callback. Because that wasn't done, the timer only fired once 1 second
after starting the plugin (when there is unlikely any data yet), and
then never again.

The timer is now re-armed in the callback to make it fire repeatedly
every second(ish). A macro is used to make sure the initial and re-arm
time interval is the same.

Broken since 4f639231.

Signed-off-by: Corubba Smith <corubba@gmx.de>
---
 output/ipfix/ulogd_output_IPFIX.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/output/ipfix/ulogd_output_IPFIX.c b/output/ipfix/ulogd_output_IPFIX.c
index 1c0f730..c1fe1ab 100644
--- a/output/ipfix/ulogd_output_IPFIX.c
+++ b/output/ipfix/ulogd_output_IPFIX.c
@@ -83,6 +83,8 @@ static const struct config_keyset ipfix_kset = {
 	}
 };

+#define SEND_TIMER_INTERVAL_SEC	1
+
 struct ipfix_priv {
 	struct ulogd_fd ufd;
 	uint32_t seqno;
@@ -259,6 +261,8 @@ static void ipfix_timer_cb(struct ulogd_timer *t, void *data)
 		priv->msg = NULL;
 		send_msgs(pi);
 	}
+
+	ulogd_add_timer(&priv->timer, SEND_TIMER_INTERVAL_SEC);
 }

 static int ipfix_configure(struct ulogd_pluginstance *pi, struct ulogd_pluginstance_stack *stack)
@@ -394,8 +398,8 @@ static int ipfix_start(struct ulogd_pluginstance *pi)
 	if (ulogd_register_fd(&priv->ufd) < 0)
 		return ULOGD_IRET_ERR;

-	/* Add a 1 second timer */
-	ulogd_add_timer(&priv->timer, 1);
+	/* Start the repeating send timer */
+	ulogd_add_timer(&priv->timer, SEND_TIMER_INTERVAL_SEC);

 	return ULOGD_IRET_OK;
 }
--
2.48.1