[libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[Evgen Bendyak]

This patch addresses a bug that occurs when the nflog_open function is
called concurrently from different threads within an application. The
function nflog_open internally invokes nflog_open_nfnl. Within this
function, a static global variable pkt_cb (static struct nfnl_callback
pkt_cb) is used. This variable is assigned a pointer to a newly
created structure (pkt_cb.data = h;) and is passed to
nfnl_callback_register. The issue arises with concurrent execution of
pkt_cb.data = h;, as only one of the simultaneously created
nflog_handle structures is retained due to the callback function.
Subsequently, the callback function __nflog_rcv_pkt is invoked for all
the nflog_open structures, but only references one of them.
Consequently, the callbacks registered by the end-user of the library
through nflog_callback_register fail to trigger in sessions where the
incorrect reference was recorded.
This patch corrects this behavior by creating the structure locally on
the stack for each call to nflog_open_nfnl. Since the
nfnl_callback_register function simply copies the data into its
internal structures, there is no need to retain pkt_cb beyond this
point.


*** a/src/libnetfilter_log.c    2024-04-30 12:45:41.974918256 +0300
--- b/src/libnetfilter_log.c    2024-04-30 12:49:56.774643783 +0300
*************** static int __nflog_rcv_pkt(struct nlmsgh
*** 161,171 ****
      return gh->cb(gh, nfmsg, &nfldata, gh->data);
  }

- static struct nfnl_callback pkt_cb = {
-     .call         = &__nflog_rcv_pkt,
-     .attr_count     = NFULA_MAX,
- };
-
  /* public interface */

  struct nfnl_handle *nflog_nfnlh(struct nflog_handle *h)
--- 161,166 ----
*************** struct nflog_handle *nflog_open_nfnl(str
*** 255,260 ****
--- 250,259 ----
  {
      struct nflog_handle *h;
      int err;
+     struct nfnl_callback pkt_cb = {
+         .call         = &__nflog_rcv_pkt,
+         .attr_count     = NFULA_MAX,
+     };

      h = calloc(1, sizeof(*h));
      if (!h)

Re: [libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[Phil Sutter]

Evgen,

On Tue, Apr 30, 2024 at 01:18:29PM +0300, Evgen Bendyak wrote:
> This patch addresses a bug that occurs when the nflog_open function is
> called concurrently from different threads within an application. The
> function nflog_open internally invokes nflog_open_nfnl. Within this
> function, a static global variable pkt_cb (static struct nfnl_callback
> pkt_cb) is used. This variable is assigned a pointer to a newly
> created structure (pkt_cb.data = h;) and is passed to
> nfnl_callback_register. The issue arises with concurrent execution of
> pkt_cb.data = h;, as only one of the simultaneously created
> nflog_handle structures is retained due to the callback function.
> Subsequently, the callback function __nflog_rcv_pkt is invoked for all
> the nflog_open structures, but only references one of them.
> Consequently, the callbacks registered by the end-user of the library
> through nflog_callback_register fail to trigger in sessions where the
> incorrect reference was recorded.
> This patch corrects this behavior by creating the structure locally on
> the stack for each call to nflog_open_nfnl. Since the
> nfnl_callback_register function simply copies the data into its
> internal structures, there is no need to retain pkt_cb beyond this
> point.

Patch looks sane, but I fear formatting won't do. Are you able to turn
this into a git commit and use git-format-patch/git-send-email to submit
it?

Thanks, Phil

Re: [libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[Evgen Bendyak]

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

The patch file in the required format has been attached to the email.

Thanks, Evgen

вт, 30 квіт. 2024 р. о 13:54 Phil Sutter <phil@nwl.cc> пише:
>
> Evgen,
>
> On Tue, Apr 30, 2024 at 01:18:29PM +0300, Evgen Bendyak wrote:
> > This patch addresses a bug that occurs when the nflog_open function is
> > called concurrently from different threads within an application. The
> > function nflog_open internally invokes nflog_open_nfnl. Within this
> > function, a static global variable pkt_cb (static struct nfnl_callback
> > pkt_cb) is used. This variable is assigned a pointer to a newly
> > created structure (pkt_cb.data = h;) and is passed to
> > nfnl_callback_register. The issue arises with concurrent execution of
> > pkt_cb.data = h;, as only one of the simultaneously created
> > nflog_handle structures is retained due to the callback function.
> > Subsequently, the callback function __nflog_rcv_pkt is invoked for all
> > the nflog_open structures, but only references one of them.
> > Consequently, the callbacks registered by the end-user of the library
> > through nflog_callback_register fail to trigger in sessions where the
> > incorrect reference was recorded.
> > This patch corrects this behavior by creating the structure locally on
> > the stack for each call to nflog_open_nfnl. Since the
> > nfnl_callback_register function simply copies the data into its
> > internal structures, there is no need to retain pkt_cb beyond this
> > point.
>
> Patch looks sane, but I fear formatting won't do. Are you able to turn
> this into a git commit and use git-format-patch/git-send-email to submit
> it?
>
> Thanks, Phil

[-- Attachment #2: 0001-fix-bug-in-race-condition-of-calling-nflog_open-from.patch --]
[-- Type: text/x-patch, Size: 1105 bytes --]

From e62369954dcb7315b738346cc5ebff89cbe3bf56 Mon Sep 17 00:00:00 2001
From: Evgenii Bendyak <jman.box@gmail.com>
Date: Tue, 30 Apr 2024 16:51:53 +0300
Subject: [PATCH] fix bug in race condition of calling nflog_open from
 different threads at same time

---
 src/libnetfilter_log.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/libnetfilter_log.c b/src/libnetfilter_log.c
index cb09384..339c961 100644
--- a/src/libnetfilter_log.c
+++ b/src/libnetfilter_log.c
@@ -161,11 +161,6 @@ static int __nflog_rcv_pkt(struct nlmsghdr *nlh, struct nfattr *nfa[],
 	return gh->cb(gh, nfmsg, &nfldata, gh->data);
 }
 
-static struct nfnl_callback pkt_cb = {
-	.call 		= &__nflog_rcv_pkt,
-	.attr_count 	= NFULA_MAX,
-};
-
 /* public interface */
 
 struct nfnl_handle *nflog_nfnlh(struct nflog_handle *h)
@@ -255,6 +250,10 @@ struct nflog_handle *nflog_open_nfnl(struct nfnl_handle *nfnlh)
 {
 	struct nflog_handle *h;
 	int err;
+	struct nfnl_callback pkt_cb = {
+		.call 		= &__nflog_rcv_pkt,
+		.attr_count 	= NFULA_MAX,
+	};
 
 	h = calloc(1, sizeof(*h));
 	if (!h)
-- 
2.25.1

Re: [libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[Phil Sutter]

On Tue, Apr 30, 2024 at 04:58:41PM +0300, Evgen Bendyak wrote:
> The patch file in the required format has been attached to the email.

Patch applied after adding your previous mail as description and an SoB
based on your From: address.

Thanks, Phil

Re: [libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[Pablo Neira Ayuso]

On Tue, Apr 30, 2024 at 01:18:29PM +0300, Evgen Bendyak wrote:
> This patch addresses a bug that occurs when the nflog_open function is
> called concurrently from different threads within an application. The
> function nflog_open internally invokes nflog_open_nfnl. Within this
> function, a static global variable pkt_cb (static struct nfnl_callback
> pkt_cb) is used. This variable is assigned a pointer to a newly
> created structure (pkt_cb.data = h;) and is passed to
> nfnl_callback_register. The issue arises with concurrent execution of
> pkt_cb.data = h;, as only one of the simultaneously created
> nflog_handle structures is retained due to the callback function.
> Subsequently, the callback function __nflog_rcv_pkt is invoked for all
> the nflog_open structures, but only references one of them.
> Consequently, the callbacks registered by the end-user of the library
> through nflog_callback_register fail to trigger in sessions where the
> incorrect reference was recorded.
> This patch corrects this behavior by creating the structure locally on
> the stack for each call to nflog_open_nfnl. Since the
> nfnl_callback_register function simply copies the data into its
> internal structures, there is no need to retain pkt_cb beyond this
> point.

Out of curiosity: How do you use this?

There is a fanout feature to distribute packets between consumer
threads to scale up.

And I suspect you don't want packets that belong to the same flow be
handled by different threads.

Re: [libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[Evgen Bendyak]

In my firewall based on nftables, I use several different log
subsystem groups for packet capturing. This setup is used for a server
providing access to a large number of internet clients, with each
client in a separate VLAN. To expand the number of virtual networks,
QinQ technology is utilized. One group captures ARP packets (in
certain situations for new clients) coming from the network, for
further analysis. Another group captures DHCP packets sent by clients.
Also present groups for other various subsystems. These are not
heavily loaded groups in terms of packet volume. In the application
where this is processed, each group is handled by its own subsystem.
Each subsystem creates its own thread, where the relevant group for
that service is opened. Sometimes, after a restart, one group or
another would fail to function. It appeared as if data was coming
through the netlink socket, but when nflog_handle_packet was called,
the callback would not trigger. That's when I began investigating what
was wrong.

вт, 30 квіт. 2024 р. о 17:46 Pablo Neira Ayuso <pablo@netfilter.org> пише:
>
> On Tue, Apr 30, 2024 at 01:18:29PM +0300, Evgen Bendyak wrote:
> > This patch addresses a bug that occurs when the nflog_open function is
> > called concurrently from different threads within an application. The
> > function nflog_open internally invokes nflog_open_nfnl. Within this
> > function, a static global variable pkt_cb (static struct nfnl_callback
> > pkt_cb) is used. This variable is assigned a pointer to a newly
> > created structure (pkt_cb.data = h;) and is passed to
> > nfnl_callback_register. The issue arises with concurrent execution of
> > pkt_cb.data = h;, as only one of the simultaneously created
> > nflog_handle structures is retained due to the callback function.
> > Subsequently, the callback function __nflog_rcv_pkt is invoked for all
> > the nflog_open structures, but only references one of them.
> > Consequently, the callbacks registered by the end-user of the library
> > through nflog_callback_register fail to trigger in sessions where the
> > incorrect reference was recorded.
> > This patch corrects this behavior by creating the structure locally on
> > the stack for each call to nflog_open_nfnl. Since the
> > nfnl_callback_register function simply copies the data into its
> > internal structures, there is no need to retain pkt_cb beyond this
> > point.
>
> Out of curiosity: How do you use this?
>
> There is a fanout feature to distribute packets between consumer
> threads to scale up.
>
> And I suspect you don't want packets that belong to the same flow be
> handled by different threads.

Re: [libnetfilter_log] fix bug in race condition of calling nflog_open from different threads at same time

[Pablo Neira Ayuso]

Hi,

On Tue, Apr 30, 2024 at 06:25:47PM +0300, Evgen Bendyak wrote:
> In my firewall based on nftables, I use several different log
> subsystem groups for packet capturing. This setup is used for a server
> providing access to a large number of internet clients, with each
> client in a separate VLAN. To expand the number of virtual networks,
> QinQ technology is utilized. One group captures ARP packets (in
> certain situations for new clients) coming from the network, for
> further analysis. Another group captures DHCP packets sent by clients.
> Also present groups for other various subsystems. These are not
> heavily loaded groups in terms of packet volume. In the application
> where this is processed, each group is handled by its own subsystem.
> Each subsystem creates its own thread, where the relevant group for
> that service is opened. Sometimes, after a restart, one group or
> another would fail to function. It appeared as if data was coming
> through the netlink socket, but when nflog_handle_packet was called,
> the callback would not trigger. That's when I began investigating what
> was wrong.

Oh I see, this is log not queue. For some reason I considered this was
the queue subsystem instead.

Thanks for explaining.