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

VLAN friendly BPF #36

Open
TheNoButton opened this issue Oct 23, 2014 · 4 comments
Open

VLAN friendly BPF #36

TheNoButton opened this issue Oct 23, 2014 · 4 comments

Comments

@TheNoButton
Copy link

I wasn't able to log 802.1q tagged packets until I updated the packet filter:

--- a/src/passivedns.c
+++ b/src/passivedns.c
@@ -1066,7 +1066,7 @@ int main(int argc, char *argv[])
     config.inpacket = config.intr_flag = 0;
     config.dnslastchk = 0;
     //char *pconfile;
-#define BPFF "port 53"
+#define BPFF "(vlan and port 53) or (not vlan and port 53)"
     config.bpff = BPFF;
     config.logfile = "/var/log/passivedns.log";
     config.logfile_nxd = "/var/log/passivedns.log";
@frsk
Copy link
Contributor

frsk commented Oct 24, 2014

Probably better to include a note in the doc or README, rather than changing the default behaviour for all users.

@maxtors
Copy link

maxtors commented Oct 24, 2014

I agree with frsk. This is an issue that the user of this software should address themselves.

@TheNoButton
Copy link
Author

Reading here, it'd be cleaner to write port 53 or (vlan and port 53). I just felt a complied-in BPF should work for both untagged and tagged packets, especially on something like this which could very well be hanging off of a fiber tap.

http://www.christian-rossow.de/articles/tcpdump_filter_mixed_tagged_and_untagged_VLAN_traffic.php

@thus
Copy link
Collaborator

thus commented Nov 11, 2014

Sorry for entering the discussion so late. I like the idea of having one BPF to rule them all. Some users are probably missing VLAN traffic in passivedns without even knowing it. However, I feel that more testing should be done to see if this has any performance impact, before forcing this change on all the users. Thanks for pointing this out, TheNoButton!

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

No branches or pull requests

4 participants