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

[v1.13] bpf: improve handling for short packets #25739

Merged
merged 3 commits into from
Jun 16, 2023
Merged

[v1.13] bpf: improve handling for short packets #25739

merged 3 commits into from
Jun 16, 2023

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented May 29, 2023 •
edited

Manual backport for

Once this PR is merged, you can update the PR labels via:

for pr in 25159; do contrib/backporting/set-labels.py $pr done 1.13; done

or with

make add-labels BRANCH=v1.13 ISSUES=25159

@julianwiedmann julianwiedmann added kind/backports This PR provides functionality previously merged into master. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. labels May 29, 2023
@julianwiedmann julianwiedmann requested a review from a team as a code owner May 29, 2023 09:22
@julianwiedmann
Copy link
Member Author

julianwiedmann commented May 29, 2023 •
edited by maintainer-s-little-helper bot

/test-backport-1.13

Job 'Cilium-PR-K8s-1.24-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with the host firewall and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service tftp://[fd04::12]:31513/hello failed

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.24-kernel-4.19/9/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Policy  cannot be deleted

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/247/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Policy  cannot be deleted

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/260/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@julianwiedmann
Copy link
Member Author

/test-1.24-4.19

@julianwiedmann
Copy link
Member Author

/test-1.26-net-next

@julianwiedmann
Copy link
Member Author

julianwiedmann commented Jun 9, 2023 •
edited by maintainer-s-little-helper bot

/test-backport-1.13

Job 'Cilium-PR-K8s-1.21-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with IPSec and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service tftp://[fd04::11]:31338/hello failed

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.21-kernel-4.19/28/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

@julianwiedmann
Copy link
Member Author

(no-change rebase to test against the current stable branch)

@julianwiedmann
bpf: pull IPv4 header into linear data in to-netdev
ae62260 
[ upstream commit afaa82e ]

af_packet can craft packets without linear data. So make sure that we pull
the IPv4 header, as resolve_srcid_ipv4() won't do it for us (we call it
with from_host = true). Also update the comment to match current behaviour.

This aligns the IPv4 path with handle_to_netdev_ipv6().

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: also pull IP header for from-host traffic
dbc095b 
[ upstream commit 1e7fc43 ]

Don't trust that host traffic arrives with the IP header in the skb's
linear space. So have resolve_srcid_ipv*() always pull the necessary data,
without differentiating between from-netdev and from-host traffic.

This means that for to-netdev traffic we now have two places that can
handle the traffic. This is just temporary, and will go away with the next
patch.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
bpf: pass IP header to resolve_srcid_ipv*()
cb4961d 
[ upstream commit e1f24b6 ]

resolve_srcid_ipv*() is meant to return an __u32, that represents the
srcid. But it currently also validates the IP header, and returns
DROP_INVALID from that. We use this value without further checks.

Fix things up by performing the IP header validation & error handling
outside the helper.

Note that the to-netdev path already contains a revalidate_data_pull()
call, so it's safe to lose this additional one inside resolve_srcid_ipv*().

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann
Copy link
Member Author

/test-backport-1.13

@julianwiedmann
Copy link
Member Author

/test-1.26-net-next

@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 16, 2023
@joestringer joestringer merged commit ab9f831 into cilium:v1.13 Jun 16, 2023
61 checks passed
@julianwiedmann julianwiedmann deleted the v1.13-short-packet branch June 17, 2023 09:24
@gentoo-root gentoo-root mentioned this pull request Jul 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@ldelossa ldelossa ldelossa approved these changes

Assignees
No one assigned
Labels
backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@julianwiedmann @joestringer @ldelossa