Skip to content

bpf: explicitly set ttl in tunnel key#12529

Merged
borkmann merged 1 commit intomasterfrom
pr/bpf-ttl-fix
Jul 15, 2020
Merged

bpf: explicitly set ttl in tunnel key#12529
borkmann merged 1 commit intomasterfrom
pr/bpf-ttl-fix

Conversation

@borkmann
Copy link
Copy Markdown
Member

@borkmann borkmann commented Jul 15, 2020

See commit msg for details. Cc: @mskrocki

Maciej reported that when using vxlan packets we can see a TTL of 64
in the packet while for geneve packets the TTL is set to 0.

This is a kernel issue in that vxlan driver in its vxlan_xmit_one()
routine derives TTL from the route if otherwise not explicitly set
(such as by BPF tunnel key):

  ttl = ttl ? : ip4_dst_hoplimit(&rt->dst);

In geneve driver however, geneve_xmit_skb() only does the above in
non-collect_md mode, which means, if not explicitly set, the TTL will
remain 0 here.

I'll post a kernel fix separately, but simple workaround is to just
set TTL in BPF tunnel key to fixed value of 64.

tunnel_ttl is part of 4.9 bpf uapi header, so there are no issues with
backwards compat.

Reported-by: Maciej Skrocki <maciejskrocki@google.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
@borkmann borkmann added pending-review area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jul 15, 2020
@borkmann borkmann requested review from a team, brb and tgraf July 15, 2020 09:16
@borkmann
Copy link
Copy Markdown
Member Author

test-me-please

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 36.988% when pulling ed0209a on pr/bpf-ttl-fix into 7d871b2 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants