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.

Sign up for GitHub

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

Jump to bottom

pcap-linux: Obtain VLAN TPID from the kernel when available #346

Merged
merged 2 commits into from Feb 15, 2015
Merged

pcap-linux: Obtain VLAN TPID from the kernel when available #346

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
235734c
pcap-linux: Obtain VLAN TPID from the kernel when available
atzm Mar 25, 2014
84c1a33
pcap-linux: Fix the VLAN TPID handling for the readability
atzm Apr 9, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 15 additions & 5 deletions pcap-linux.c
View file
Open in desktop
Expand Up @@ -366,6 +366,12 @@ static void pcap_oneshot_mmap(u_char *user, const struct pcap_pkthdr *h,
const u_char *bytes);
#endif

#ifdef TP_STATUS_VLAN_TPID_VALID
# define VLAN_TPID(hdr, hv) (((hv)->tp_vlan_tpid || ((hdr)->tp_status & TP_STATUS_VLAN_TPID_VALID)) ? (hv)->tp_vlan_tpid : ETH_P_8021Q)
Copy link

Choose a reason for hiding this comment

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

Should this be as following?

# define VLAN_TPID(hdr, hv) ((((hv)->tp_vlan_tpid != 0) && ((hdr)->tp_status & TP_STATUS_VLAN_TPID_VALID)) ? (hv)->tp_vlan_tpid : ETH_P_8021Q)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the tp_vlan_tpid is always defined as the unsigned (__u16) in TPACKET structures, that change is not needed, IMHO. And if the definition is changed to the signed in the future, I think this is a kernel's bug...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the noise, I was misreading your points.
The logic for the selecting TPID is same as the TCI, but should it be different logic only in case of the TPID? It seems pcap-linux uses the value when the kernel gives any value if TP_STATUS_*_VALID is defined.

Copy link

Choose a reason for hiding this comment

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

The reason I posted question was that second TP_STATUS_VLAN_TPID_VALID does nothing meaningful when conbined with series of past linux kernels. If a packet has tpid, then first condition will be met, because tpid won't be 0.

Anyway, vlan tag existence will be checked by vlan_tci, and not vlan_tpid. If TP_STATUS_VLAN_TPID_VALID is defined, then we need just check that bit without checking tp_vlan_tpid is 0 or not, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right. But then I think the macro just should be as following:

# define VLAN_TPID(hdr, hv) ((hdr)->tp_status & TP_STATUS_VLAN_TPID_VALID ? (hv)->tp_vlan_tpid : ETH_P_8021Q)

Of course the tp_vlan_tpid will be used in this case even if the TPID is 0, but it ought to be right in the userspace, because the kernel set the TP_STATUS_VLAN_TPID_VALID.

#else
# define VLAN_TPID(hdr, hv) ETH_P_8021Q
#endif

/*
* Wrap some ioctl calls
*/
Expand Down Expand Up @@ -1653,7 +1659,7 @@ pcap_read_packet(pcap_t *handle, pcap_handler callback, u_char *userdata)
memmove(bp, bp + VLAN_TAG_LEN, handlep->vlan_offset);

tag = (struct vlan_tag *)(bp + handlep->vlan_offset);
tag->vlan_tpid = htons(ETH_P_8021Q);
tag->vlan_tpid = htons(VLAN_TPID(aux, aux));
tag->vlan_tci = htons(aux->tp_vlan_tci);

packet_len += VLAN_TAG_LEN;
Expand Down Expand Up @@ -4168,7 +4174,8 @@ static int pcap_handle_packet_mmap(
unsigned int tp_sec,
unsigned int tp_usec,
int tp_vlan_tci_valid,
__u16 tp_vlan_tci)
__u16 tp_vlan_tci,
__u16 tp_vlan_tpid)
{
struct pcap_linux *handlep = handle->priv;
unsigned char *bp;
Expand Down Expand Up @@ -4264,7 +4271,7 @@ static int pcap_handle_packet_mmap(
memmove(bp, bp + VLAN_TAG_LEN, handlep->vlan_offset);

tag = (struct vlan_tag *)(bp + handlep->vlan_offset);
tag->vlan_tpid = htons(ETH_P_8021Q);
tag->vlan_tpid = htons(tp_vlan_tpid);
tag->vlan_tci = htons(tp_vlan_tci);

pcaphdr.caplen += VLAN_TAG_LEN;
Expand Down Expand Up @@ -4324,6 +4331,7 @@ pcap_read_linux_mmap_v1(pcap_t *handle, int max_packets, pcap_handler callback,
h.h1->tp_sec,
h.h1->tp_usec,
0,
0,
0);
if (ret == 1) {
pkts++;
Expand Down Expand Up @@ -4402,7 +4410,8 @@ pcap_read_linux_mmap_v2(pcap_t *handle, int max_packets, pcap_handler callback,
#else
h.h2->tp_vlan_tci != 0,
#endif
h.h2->tp_vlan_tci);
h.h2->tp_vlan_tci,
VLAN_TPID(h.h2, h.h2));
if (ret == 1) {
pkts++;
handlep->packets_read++;
Expand Down Expand Up @@ -4497,7 +4506,8 @@ pcap_read_linux_mmap_v3(pcap_t *handle, int max_packets, pcap_handler callback,
#else
tp3_hdr->hv1.tp_vlan_tci != 0,
#endif
tp3_hdr->hv1.tp_vlan_tci);
tp3_hdr->hv1.tp_vlan_tci,
VLAN_TPID(tp3_hdr, &tp3_hdr->hv1));
if (ret == 1) {
pkts++;
handlep->packets_read++;
Expand Down