Bug #1524: Potential Thread Name issues due to RHEL7 Interface Naming Contentions - Suricata - Open Information Security Foundation

Actions

Copy link

Bug #1524

closed

Potential Thread Name issues due to RHEL7 Interface Naming Contentions

Added by Zach Rasmor over 8 years ago. Updated almost 8 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Zach Rasmor

Target version:

3.1rc1

Affected Versions:

Effort:

Difficulty:

Label:

Description

I have begun testing Suricata on RHEL7, on an interface named "enp132s0." The length of this interface name resulted in the following thread names for my 16 threads:

RxPcapenp132s01
 RxPcapenp132s02
 RxPcapenp132s03
 RxPcapenp132s04
 RxPcapenp132s05
 RxPcapenp132s06
 RxPcapenp132s07
 RxPcapenp132s08
 RxPcapenp132s09
 RxPcapenp132s01 <--
 RxPcapenp132s01 <--
 RxPcapenp132s01 <--
 RxPcapenp132s01 <--
 RxPcapenp132s01 <--
 RxPcapenp132s01 <--
 RxPcapenp132s01 <--

Furthermore, the stats log only displayed threads 1-9.

$ grep flow /var/log/suricata/stats.log | tail -20
 flow.emerg_mode_entered   | FlowManagerThread         | 0
 flow.emerg_mode_over      | FlowManagerThread         | 0
 flow.tcp_reuse            | FlowManagerThread         | 0
 tcp.no_flow               | RxPcapenp132s01           | 0
 tcp.no_flow               | RxPcapenp132s02           | 0
 tcp.no_flow               | RxPcapenp132s03           | 0
 tcp.no_flow               | RxPcapenp132s04           | 0
 tcp.no_flow               | RxPcapenp132s05           | 0
 tcp.no_flow               | RxPcapenp132s06           | 0
 tcp.no_flow               | RxPcapenp132s07           | 0
 tcp.no_flow               | RxPcapenp132s08           | 0
 tcp.no_flow               | RxPcapenp132s09           | 0
 flow_mgr.closed_pruned    | FlowManagerThread         | 0
 flow_mgr.new_pruned       | FlowManagerThread         | 31431
 flow_mgr.est_pruned       | FlowManagerThread         | 0
 flow.memuse               | FlowManagerThread         | 464960416
 flow.spare                | FlowManagerThread         | 1050507
 flow.emerg_mode_entered   | FlowManagerThread         | 0
 flow.emerg_mode_over      | FlowManagerThread         | 0
 flow.tcp_reuse            | FlowManagerThread         | 6

This truncation in the thread names can be traced back to the following:

From tm-threads.h:
#define TM_QUEUE_NAME_MAX 16
#define TM_THREAD_NAME_MAX 16

And from util-runmodes.c, RunModeSetLiveCaptureWorkersForDevice:
for (thread = 0; thread < threads_count; thread++) {
char tname[TM_THREAD_NAME_MAX];
...
snprintf(tname, sizeof(tname), "%s%s%"PRIu16, thread_name, live_dev, thread+1);

I can certainly look into changing the name of my interface to something shorter, but I figured this was worth mentioning since RHEL7 has adopted a new interface naming convention, and this seems to cause issues with stats reporting.

Perhaps the fix is as simple as raising the #define MAX's to 32, but I wasn't sure if that would have unwanted consequences.

Actions

Copy link

Updated by Victor Julien over 8 years ago

Target version set to 3.0RC1

Some works has been done to address this, but it's not yet complete. Latest iteration is at https://github.com/inliniac/suricata/pull/1466. As Andreas doesn't seem to be working on it, perhaps you can pick it up?

Actions

Copy link

Updated by Zach Rasmor over 8 years ago

Sure, I can take a look at what is done and move it forward.

Two questions:
1. Though I haven't looked at the code in detail yet, I noticed that the PR you referenced is closed without comments. Do you recall what the remaining issues were?
2. What is the process for augmenting another contributor's work - fork his branch and open my own pull request with my commits on top of his?

Actions

Copy link

Updated by Victor Julien over 8 years ago

1) The feedback was given in IRC, here are my logs:

maxmo, I think I see the issue: the thread name is directly assigned to 'tv->name'. So tv->name now points to a variable that goes out of scope
maxmo, previously it pointed to a string constant
maxmo, quick and dirty would be passing the result of strdup(name) to TmThreadCreateMgmtThreadByName
maxmo, if you're interesting in doing a bit more work, you could change the tv::name ptr into a small fixed size string

2) You can take the branch, rebase it to my master and then make additional changes in new commits. So yeah, fork his branch and add your own commits.

Actions

Copy link

Updated by Zach Rasmor over 8 years ago

Assignee set to Zach Rasmor

Actions

Copy link

Updated by Zach Rasmor over 8 years ago

Hello Victor, I wanted to confirm a couple of things:

In the dicussion on https://github.com/inliniac/suricata/pull/1355, you suggested the format "W#12 => worker thread 12 (workers and non-capture threads in autofp)"

Regarding Autofp, the current name for non-capture threads is "Detect"...just wanted to see if you still preferred it shortened to "W#-iface" (as you stated above) or if you'd rather have it as "D#-iface" to closer resemble the current name?

Also, for single mode, do you have a preference between "C#-iface", "W#-iface", or something else? I was thinking "W" might make more sense here.

Actions

Copy link

Updated by Victor Julien over 8 years ago

Status changed from New to Assigned

For autofp I prefer "W" as well, as it's not just doing detect, but also stream tracking/reassembly and the outputs.

ACK on single: single is really the same as 'workers', except there is just one thread and not multiple.

On the IPS side we haven't figured things out yet. There we have a verdict/transmit thread as well. Maybe capture should be RX and verdict/transmit should be TX.

Something like:
RX#1-eth0
TX#1-eth0
W#1
W#2
FM#1

Actions

Copy link