Skip to content

Commit

Permalink
fix(network-monitor): Include the packet payload regarless of L4 prot…
Browse files Browse the repository at this point in the history
…ocol

* User-space part of pulsar is expecting the buffer to be initialized
  regardless of L4 protocol (TCP or UDP).
* DNS responses, if long enough, might go through TCP. Not very common,
  but possible.
* Stop returning an error when packet payload length is 0. It's totally
  normal for SYN, SYN-ACK and ACK packets. Logging errors for them is
  an unnecessary noise.

Fixes #297
  • Loading branch information
vadorovsky committed Jun 24, 2024
1 parent b957fe0 commit c783846
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 11 deletions.
12 changes: 9 additions & 3 deletions crates/bpf-builder/include/buffer.bpf.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,16 @@ static __always_inline int buffer_append_skb_bytes(struct buffer *buffer,
return -1;
}

// * The `< 0` case is impossible to reproduce (otherwise it would mean
// `skb->len` is somehow lower than the size of packet headers we already
// consumed from that SKB).
// It's here only to appease the eBPF verifier (using `u32` and checking
// just for `0` doesn't work).
// * The `= 0` case usually applies to SYN, SYN-ACK, ACK or any other packets
// which don't contain any payload after L4 headers.
// Therefore, we don't return any error here.
if (len <= 0) {
LOG_ERROR("Invalid offset (%zu) exceeding the packet length (%zu).",
offset, skb->len);
return -1;
return 0;
}

int r = bpf_skb_load_bytes(skb, offset, &((char *)buffer->buffer)[pos],
Expand Down
13 changes: 7 additions & 6 deletions crates/modules/network-monitor/probes.bpf.c
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,8 @@ __always_inline int process_skb(struct __sk_buff *skb,
void *data_end = (void *)(long)skb->data_end;
void *data = (void *)(long)skb->data;

buffer_index_init(&network_event->buffer, &msg_event->data);

// Parse L3 header (IPv4 / IPv6).
switch (l3_proto) {
case ETH_P_IPV4: {
Expand Down Expand Up @@ -653,19 +655,18 @@ __always_inline int process_skb(struct __sk_buff *skb,
}
break;
}

buffer_index_init(&network_event->buffer, &msg_event->data);
if (buffer_append_skb_bytes(&network_event->buffer, &msg_event->data, skb,
headers_len) < 0) {
LOG_ERROR("Failed to retrieve the packet payload. The event is going to miss the `data` part.");
}
break;
}
default:
LOG_DEBUG("ignored unsupported L4 protocol %d", l4_proto);
goto send_event;
}

if (buffer_append_skb_bytes(&network_event->buffer, &msg_event->data, skb,
headers_len) < 0) {
LOG_ERROR("Failed to retrieve the packet payload. The event is going to miss the `data` part.");
}

msg_event->data_len = skb->len - headers_len;

send_event:
Expand Down
4 changes: 2 additions & 2 deletions crates/modules/network-monitor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,11 +303,11 @@ pub mod pulsar {
let data = data
.bytes(&event.buffer)
.map_err(|err| {
log::error!("[dns] Error getting message: {}", err);
log::error!("Error getting network packet payload: {err}");
})
.ok()?;

// any valid dns data?
// Check wheter the payload contains any DNS data.
let dns = dns_parser::Packet::parse(data).ok()?;
let with_q = !dns.questions.is_empty();
let with_a = !dns.answers.is_empty();
Expand Down

0 comments on commit c783846

Please sign in to comment.