Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC Feature/60x/decoder data store/v8 #4977

Closed

Conversation

victorjulien
Copy link
Member

Initial work on 'decoder store', where the packet fields stored in the Packet are more dynamic.

From efaf3bc
A fixed size static store is part of the Packet structure. If that space
isn't sufficient the store can dynamically expand. It uses a list of
storage blocks for this.

The primary goals are:

  • don't use space in the packet when a feature/proto isn't in use
  • reduce (or avoid) the need for new protocols to manually add to the
    Packet structure

In general the storage is a TLV (type length value) store, but some types
get special support to reduce the space overhead of the TLV header.

Implemented so far:

  • VLAN (2 bytes)
  • MPLS (4 bytes)
  • TLV (3 bytes + the data to be stored)

Right now the only 'consumer' is EVE, where the current PR adds

  "decoder_store": [
    {
      "mac": "52:54:00:12:35:02"
    },
    {
      "mac": "08:00:27:bf:4f:8a"
    },
    {
      "vlan": 666
    },
    {
      "vlan": 4094
    }
  ],

and

  "decoder_store": [
    {
      "mac": "00:00:00:06:fd:01"
    },
    {
      "mac": "17:73:74:61:74:65"
    },
    {
      "mpls": 291956
    }
  ],

Looking for general feedback. Also wondering if this should be extended to the Flow, and perhaps somehow even be taken into account for flow hash lookups.

A fixed size static store is part of the Packet structure. If that space
isn't sufficient the store can dynamically expand. It uses a list of
storage blocks for this.

The primary goals are:
- don't use space in the packet when a feature/proto isn't in use
- reduce (or avoid) the need for new protocols to manually add to the
  Packet structure

In general the storage is a TLV (type length value) store, but some types
get special support to reduce the space overhead of the TLV header.

Implemented so far:
- VLAN (2 bytes)
- MPLS (4 bytes)
- TLV (3 bytes + the data to be stored)
@victorjulien victorjulien mentioned this pull request May 25, 2020
3 tasks
@jasonish
Copy link
Member

Looking for general feedback. Also wondering if this should be extended to the Flow, and perhaps somehow even be taken into account for flow hash lookups.

I think its important to have this same information in flow records as well. Not sure I like how generic this is to get at this fields under the decoder_store. I also feel like that "decoder_store" name should be an internal implementation detail and not be exposed in the JSON. Is there a better name we can use here?

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass it looks OK. As mentioned, not too keen on the output, but I see why its like that given the way things are stored. Need to give it a second pass and think on it a bit.

}


typedef union DecodeStoreBuiltinVLAN {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not liking the "Builtin" in the name too much. "DecoderStoreTypeVlan", "DecodeStoreTypeGeneric", etc.

{
assert(size >= 1);

*type = *data & 0x0f;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will 16 enough for types ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea was that there are some built-in types and then a flexible general purpose TLV type that allows 256 subtypes. The builtins should be extra compact/efficient.

@catenacyber
Copy link
Contributor

What is the status on this ?
To be closed ?

@catenacyber catenacyber closed this Jan 6, 2022
victorjulien added a commit to victorjulien/suricata that referenced this pull request Jan 27, 2023
Inspect individual chunks in lossy traffic.

Don't use the frame idx as the inspection buffer idx. Engines are running
per frame, so multi inspection can be used for stream chunks instead.

Ticket: OISF#4977.
victorjulien added a commit to victorjulien/suricata that referenced this pull request Jan 28, 2023
Inspect individual chunks in lossy traffic.

Don't use the frame idx as the inspection buffer idx. Engines are running
per frame, so multi inspection can be used for stream chunks instead.

Ticket: OISF#4977.
victorjulien added a commit to victorjulien/suricata that referenced this pull request Jan 28, 2023
Inspect individual chunks in lossy traffic.

Don't use the frame idx as the inspection buffer idx. Engines are running
per frame, so multi inspection can be used for stream chunks instead.

Ticket: OISF#4977.
@satta
Copy link
Contributor

satta commented Sep 7, 2023

Bump -- this would be interesting/useful to me as well as I'm looking at MPLS logging atm and I already felt guilty for adding more fields to the packet and flow structs in my branch (https://github.com/satta/suricata/tree/mpls) ;)

I agree that the output should be different, more in line with current EVE structure and not expose how things are stored.

@satta
Copy link
Contributor

satta commented Oct 2, 2023

I am also still curious what the status is here.

@victorjulien
Copy link
Member Author

@satta, not sure. I think it needs a rebase and I also really dislike the macro hack (TOP/BOT) think I did... feel free to play around with it.

@satta
Copy link
Contributor

satta commented Oct 3, 2023

Yeah, quite a big rebase as quite a bit seems to have changed in the packet department. I'll see when i can find the time to do the rebase (and hopefully get a better idea of the change in the process).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants