-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
af-packet: terminate on same iface and copyiface #9308
Conversation
If the interface and copy-iface are same for an af-packet IPS device setting then fataly exit. Bug 5870
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be enough to check in ParseAFPConfig
if strcmp(iface, out_iface) == 0
right after
if (ConfGetChildValueWithDefault(if_root, if_default, "copy-iface", &out_iface) == 1) {
if (strlen(out_iface) > 0) {
aconf->out_iface = out_iface;
}
}
?
TAILQ_FOREACH (child, &base->head, next) { | ||
ConfNode *subchild; | ||
const char *iface = NULL, *copyiface = NULL; | ||
TAILQ_FOREACH (subchild, &child->head, next) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand what we're looping here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, each of the entries in a list item like
- interface: wlp0s20f3
threads: auto
cluster-id: 99
cluster-type: cluster_flow
defrag: yes
copy-mode: ips
copy-iface: eth2
@@ -271,6 +271,36 @@ const char *LiveGetShortName(const char *dev) | |||
return live_dev->dev_short; | |||
} | |||
|
|||
int CheckAFPacketIPSDevs(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this in util-device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that since we're looping for ifaces and that kinda logic seemed to already be here in this file so this might be the right place.
@@ -1024,6 +1024,10 @@ static TmEcode ParseInterfacesList(const int runmode, char *pcap_dev) | |||
SCLogError("No interface found in config for af-packet"); | |||
SCReturnInt(TM_ECODE_FAILED); | |||
} | |||
int retval = CheckAFPacketIPSDevs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be checked in the af-packet files instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. As per your main suggestion since all of the other things done are invalid, it should all probably be there.
WARNING:
Pipeline 15458 |
Please note that this works but the program exits much later than the approach in this PR.
vs this PR:
Is this OK? |
Can you explain why this happens? |
For the following conf
It seems to me that with this PR,
Note: However, now, I'm thinking this is probably wrong bc we haven't established if it's the AF_PACKET IPS mode yet. With your approach, I saw that a lot of things happen before we get to checking the configuration.
It seems to me that this is the correct approach indeed as this happens after we have established that we're dealing with the correct modes (AF_PACKET IPS in this case). |
Thanks for the analysis, I think it explains the issue well. I still think the place I suggested is the correct place to check this, however it does seem inefficient. I wonder if it makes sense to rework the initialization to do more of the config validation work earlier. Perhaps a 2 step approach, where parsing happens early, but actual start of the runtime late. Anyway, I think we can take the simple approach for now, and create a future work ticket for the optimizations. |
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5870
Previous PR: #9307
Changes since v2: