Project

General

Profile

Actions

Optimization #6572

open

Bug #5711: runmodes: Suricata does not hint anything about missing runmode

runmodes: fix `--list-runmodes` output

Added by Juliana Fajardini Reichow about 1 year ago. Updated 5 months ago.

Status:
New
Priority:
Normal
Target version:
Effort:
Difficulty:
Label:
Beginner, Good First Issue

Description

As raised on https://github.com/OISF/suricata/pull/9743#discussion_r1386115590
the output of `--list-runmodes` isn't very useful at the moment.

We must consider what should be shown for that command.

Actions #1

Updated by Comfort Amaechi about 1 year ago

If I may ask, what specific aspects of the current --list-runmodes output that you find lacking or could be improved?
Could you provide more details on the likely information or format you envision for the improved --list-runmodes output

Actions #2

Updated by Victor Julien about 1 year ago

How about something like:

# suricata --list-runmodes

pcap:
  default runmode: autofp
  supported runmodes: single, autofp, workers
  description: libpcap based capture method
  notes: workers is only useful with NIC specific libpcap implementation supporting multi-capture (see XXX)
  yaml section: pcap
  commandline: --pcap or --pcap=<dev>
  supported features: none
af-packet:
  default runmode: workers
  supported runmodes: single, autofp, workers
  ...
  notes: on Linux -i uses af-packet by default
  supported features: ips csum_offload

Actions #3

Updated by Comfort Amaechi about 1 year ago

Thank you for providing the suggested format. The proposed structure provides a clear and detailed overview of each runmode. Also Could you please provide insights on the preferred format for this output? For example, should it follow a tabular structure like the initial output or a different format?

Actions #4

Updated by Juliana Fajardini Reichow about 1 year ago

Comfort Amaechi wrote in #note-3:

Thank you for providing the suggested format. The proposed structure provides a clear and detailed overview of each runmode. Also Could you please provide insights on the preferred format for this output? For example, should it follow a tabular structure like the initial output or a different format?

I would say follow the example Victor has given, I believe that's similar to what we currently have, right?

Actions #5

Updated by Lukas Sismis 10 months ago

  • Label Beginner, Good First Issue added
Actions #6

Updated by Gabriel Lima Luz 7 months ago

Hello, as I commented in the ticket #5711, I think it would be better to start working on this first. I would like to claim this ticket.
The Documentation contains information about the runmodes for me to base what the command message will say?

Actions #7

Updated by Juliana Fajardini Reichow 7 months ago

Gabriel Lima Luz wrote in #note-6:

Hello, as I commented in the ticket #5711, I think it would be better to start working on this first. I would like to claim this ticket.

Hello, indeed, this one should be finished first. Please, feel free to assign it to yourself :)

The Documentation contains information about the runmodes for me to base what the command message will say?

For starters, I would see what is the current output, then try to reformat it to follow Victor's suggestion on the comment above, and then you can share a draft PR and we can see what's missing.
For documentation, I'm afraid it may be a bit incomplete - if that's truly the case, this could be also part of this task, once the code part is done: updating the documentation, so all runmodes are at least listed there.

This would be what we have, for now: https://docs.suricata.io/en/latest/performance/runmodes.html

Actions #8

Updated by Gabriel Lima Luz 7 months ago

  • Assignee changed from OISF Dev to Gabriel Lima Luz
Actions #9

Updated by Gabriel Lima Luz 7 months ago · Edited

the function responsable to print the message is the RunModeRegisterRunModes in the rundmodes.c file :

void RunModeListRunmodes(void)
{
    printf("------------------------------------- Runmodes -------------------" 
           "-----------------------\n");

    printf("| %-17s | %-17s | %-10s \n",
           "RunMode Type", "Custom Mode ", "Description");
    printf("|-----------------------------------------------------------------" 
           "-----------------------\n");
    int i = RUNMODE_UNKNOWN + 1;
    int j = 0;
    for ( ; i < RUNMODE_USER_MAX; i++) {
        int mode_displayed = 0;
        for (j = 0; j < runmodes[i].cnt; j++) {
            if (mode_displayed == 1) {
                printf("|                   ----------------------------------------------" 
                       "-----------------------\n");
                RunMode *runmode = &runmodes[i].runmodes[j];
                printf("| %-17s | %-17s | %-27s \n",
                       "",
                       runmode->name,
                       runmode->description);
            } else {
                RunMode *runmode = &runmodes[i].runmodes[j];
                printf("| %-17s | %-17s | %-27s \n",
                       RunModeTranslateModeToName(runmode->runmode),
                       runmode->name,
                       runmode->description);
            }
            if (mode_displayed == 0)
                mode_displayed = 1;
        }
        if (mode_displayed == 1) {
            printf("|-----------------------------------------------------------------" 
                   "-----------------------\n");
        }
    }


from what I understand it prints info from a list of items of the type RunMode which is defined like this:
typedef struct RunMode_ {
    /* the runmode type */
    enum RunModes runmode;
    const char *name;
    const char *description;
    /* runmode function */
    int (*RunModeFunc)(void);
    int (*RunModeIsIPSEnabled)(void);
} RunMode;

So my question is to make the function follow the format suggested above what would be the best approach ? should I change the the struct fields and add something similar to the message or just change what the RunmMode->description contains ?

Actions #10

Updated by Juliana Fajardini Reichow 7 months ago

Gabriel Lima Luz wrote in #note-9:

So my question is to make the function follow the format suggested above what would be the best approach ? should I change the the struct fields and add something similar to the message or just change what the RunmMode->description contains ?

I think that what we can do is add more fields to the RunMode struct and populate these.
For a first PR, it is ok (imho) if you just provide a proposal of what the struct would look like, where/how these fields would be handled, and what the RunModeListRunmodes function would look like, and once that is settled, to move onto to populate these extra fields themselves.
So, steps and tasks that I see:
- proposing new structure fields to account for Victor's suggestion
- propose how/where those will be initialized
- propose what the RunModeListRunmodes will look like
- populate the approved new structure fields, for each run-mode
- update documentation as needed

(I may be missing stuff, but it's good to list what we already foresee)

Actions #11

Updated by Gabriel Lima Luz 6 months ago

Hi.
I was looking into the old output from the command and there is a PCAP_DEV and a PCAP_FILE, I'm a little confused in what is the difference between the two runmodes and how to describe it the new format of the command

Actions #12

Updated by Victor Julien 6 months ago

PCAP_DEV = libpcap live mode, so on an interface like eth0
PCAP_FILE = libpcap offline mode, so read a pcap file.

Actions #13

Updated by Gabriel Lima Luz 6 months ago · Edited

Ok, based on your answer I got two questions.
First one: the command should output a text describing the PCAP_FILE and another for the PCAP_DEV ?
Second question: The information displayed by the command comes from the RunMode Struct and the fields are initialized in the RunModeRegisterNewRunMode function. this function is being called in every file that defines the runmodes in suricata. for example in the ruunmode-pcap.c we have:

 void RunModeIdsPcapRegister(void)
 {
     RunModeRegisterNewRunMode(RUNMODE_PCAP_DEV, "single", "Single threaded pcap live mode",
             RunModeIdsPcapSingle, NULL);
     RunModeRegisterNewRunMode(RUNMODE_PCAP_DEV, "autofp",
             "Multi-threaded pcap live mode. Packets from each flow are assigned to a consistent " 
             "detection thread",
             RunModeIdsPcapAutoFp, NULL);
     RunModeRegisterNewRunMode(RUNMODE_PCAP_DEV, "workers",
             "Workers pcap live mode, each thread does all" 
             " tasks from acquisition to logging",
             RunModeIdsPcapWorkers, NULL);
 }

From what I understand we have three call, one for single, autofp and workers runmode. Since we are aiming to add the supported runmodes field we could modify both the struct and function to add that parameter and field and eliminate the other two calls, is that ok ? does removing the others RunModeRegisterNewRunMode call with make another impacts to the code ?

Actions #14

Updated by Juliana Fajardini Reichow 5 months ago

Hi there, first of all, apologies for the long time to answer.

Gabriel Lima Luz wrote in #note-13:

Ok, based on your answer I got two questions.
First one: the command should output a text describing the PCAP_FILE and another for the PCAP_DEV ?

Yes, as they are different run modes, expecting different parameter types and that, to some extent, will have different behaviors.

Second question: The information displayed by the command comes from the RunMode Struct and the fields are initialized in the RunModeRegisterNewRunMode function. this function is being called in every file that defines the runmodes in suricata. for example in the ruunmode-pcap.c we have:
[...]
From what I understand we have three call, one for single, autofp and workers runmode. Since we are aiming to add the supported runmodes field we could modify both the struct and function to add that parameter and field and eliminate the other two calls, is that ok ? does removing the others RunModeRegisterNewRunMode call with make another impacts to the code ?

I'm not sure I understand the suggestion.
Our goal is to fix the output of the function RunModeListRunmodes. To achieve that, we likely will need to add extra fields to the RunMode struct, to add the new info we want to show (e.g. the notes field). We currently have an array of RunMode@s that will store each supported @RunMode.

In my interpretation (that can change, ofc), the current model makes it evident that we are registering different supported modes with each call, and allows us to register or move through the supported RunModes in a more compact way (by using the for loops that you see in the code).

So I give the question back to you :P Apart from saving some extra calls to RunModeRegisterNewRunMode, how would that benefit the rest of the implementation and logic? Wouldn't that make registering each supported run mode more difficult?

Actions

Also available in: Atom PDF