support xdp auth with tail call #869

weli-l · 2024-09-19T07:36:46Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kmesh-bot · 2024-09-19T07:36:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kevin-wangzefeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

LiZhenCheng9527 · 2024-09-19T07:57:33Z

bpf/kmesh/workload/include/authz.h

@@ -22,6 +23,25 @@ struct {
    __uint(max_entries, MAP_SIZE_OF_AUTH_POLICY);
 } map_of_authz SEC(".maps");

+struct match_result {


Is this the same content as PR #863 ?
Can't we put them together?

Is this the same content as PR #863 ? Can't we put them together?

PR #863 is relatively large and contains the tail call logic of this PR. Merge this PR first and close the previous PR #863

Then you may need to add the /hold labels to #863.

codecov · 2024-09-19T08:26:26Z

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 54.43%. Comparing base (0a7735d) to head (aa98335).
Report is 281 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/bpf/workload/xdp.go	60.00%	2 Missing and 2 partials ⚠️

Files with missing lines	Coverage Δ
pkg/bpf/workload/xdp.go	`55.38% <60.00%> (ø)`

... and 8 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99e0c0c...aa98335. Read the comment docs.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

hzxuzhonghu · 2024-09-20T01:17:58Z

Wanna to know why do we need tailcall here

weli-l · 2024-09-20T01:23:29Z

Wanna to know why do we need tailcall here

Because the subsequent authentication rules will support srcip, dstip, namespce, etc., but if tailcall is not used, the bytes of the ebpf program will be too large (up to 1000000 bytes), and it will not pass the verification of the verifier.

hzxuzhonghu · 2024-09-20T01:57:35Z

got it

supercharge-xsy · 2024-09-20T02:41:30Z

bpf/kmesh/workload/include/authz.h

+struct match_result {
+    __u32 action;
+    __u32 match_res;
+    __u16 dport;


if dport is needed？ this can be get from element "match"

if dport is needed？ this can be get from element "match"

We need to know whether it is ipv4 or ipv6 type. If we do not pass dport, we need to pass xdp_info to determine whether it is v4 or v6.

supercharge-xsy · 2024-09-20T02:41:58Z

bpf/kmesh/workload/include/authz.h

@@ -22,6 +23,25 @@ struct {
    __uint(max_entries, MAP_SIZE_OF_AUTH_POLICY);
 } map_of_authz SEC(".maps");

+struct match_result {


Suggested change

struct match_result {

struct match_ctx {

supercharge-xsy · 2024-09-20T02:45:11Z

bpf/kmesh/workload/include/authz.h

+    __uint(key_size, sizeof(__u32));
+    __uint(value_size, sizeof(struct match_result));
+    __uint(max_entries, 1);
+} map_of_t_data SEC(".maps");


map_of_tailcall_info

map_of_tailcall_info

this can also be truncated as map_of_tailcall, so I change it to tailcall_info_map

supercharge-xsy · 2024-09-20T02:53:54Z

bpf/kmesh/workload/include/authz.h

+    struct match_result *res;
+
+    res = bpf_map_lookup_elem(&map_of_t_data, &key);
+    if (!res) {


when the first time come, can not find a record?

supercharge-xsy · 2024-09-20T02:55:07Z

bpf/kmesh/workload/include/authz.h

 {
+    struct match_result *res;
+    __u32 key = 0;


If the key is always 0, there is a concurrency problem.

supercharge-xsy · 2024-09-20T02:57:06Z

bpf/kmesh/workload/include/tail_call.h

+    __uint(value_size, sizeof(__u32));
+    __uint(max_entries, MAP_SIZE_OF_TAIL_CALL_PROG);
+    __uint(map_flags, 0);
+} map_of_tail_call_prog_for_xdp SEC(".maps");


The map name is too long and will be truncated when used bpftool

The map name is too long and will be truncated when used bpftool

change to xdp_tailcall_map

supercharge-xsy · 2024-09-20T02:58:53Z

bpf/kmesh/workload/xdp.c

-    if (xdp_rbac_manage(&info, &tuple_info) == AUTH_DENY) {
-        return xdp_deny_packet(&info, &tuple_info);
-    }
+    // tail call will be executed in it, so there will be no return value, ignore it.


line 196 is also to be processed

supercharge-xsy · 2024-09-20T03:01:47Z

It is recommended that this modification be merged after version 0.5 is released.

supercharge-xsy · 2024-10-09T03:01:12Z

bpf/kmesh/workload/include/authz.h

 {
-    __u32 matchResult;
+    __u32 key = bpf_get_smp_processor_id();
+    struct match_result res;


Signed-off-by: weli-l <[email protected]>

hzxuzhonghu · 2024-10-12T03:09:03Z

bpf/kmesh/workload/include/authz.h

@@ -22,6 +23,22 @@ struct {
    __uint(max_entries, MAP_SIZE_OF_AUTH_POLICY);
 } map_of_authz SEC(".maps");

+struct match_ctx {
+    __u32 action;


please add some comments

hzxuzhonghu

What i understand is each time a packet received, it will run auth, no considering it was done for the same connection, right?

hzxuzhonghu · 2024-10-12T03:20:22Z

bpf/kmesh/workload/include/authz.h


    if (match->n_destination_ports == 0 && match->n_not_destination_ports == 0) {
-        return MATCHED;
+        bpf_tail_call(ctx, &xdp_tailcall_map, TAIL_CALL_AUTH_IN_USER_SPACE);
+        return (res->action == AUTH_DENY) ? XDP_PASS : XDP_DROP;


should this be opposite

if bpf_tail_call succeed, it can not runinto the return

need delete the map record if tail_call failed, and other tail_call code

hzxuzhonghu · 2024-10-12T03:23:50Z

bpf/kmesh/workload/include/authz.h

+    int ret;
+
+    if (get_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {
+        BPF_LOG(ERR, AUTH, "Failed to get tuple key\n");


nit: \n is not needed

hzxuzhonghu · 2024-10-12T03:26:58Z

bpf/kmesh/workload/include/authz.h

+}
+
+SEC("xdp_auth")
+int matchDstPorts(struct xdp_md *ctx)


obey c naming convention

Suggested change

int matchDstPorts(struct xdp_md *ctx)

int match_dst_port(struct xdp_md *ctx)

how do you make sure this prog is not triggered each time a packet received

hzxuzhonghu · 2024-10-12T03:37:56Z

bpf/kmesh/workload/include/authz.h

-    // todo: add other match types
-    matchResult = matchDstPorts(match, info, tuple_info);
-    return matchResult;
+    ret = bpf_map_update_elem(&tailcall_info_map, &tuple_key, &res, BPF_ANY);


If there are multi policy set, multiple packet concurrently received, isn't this map overwritten with random policy?

Signed-off-by: weli-l <[email protected]>

supercharge-xsy · 2024-10-12T07:53:39Z

bpf/kmesh/workload/include/authz.h

+    __uint(type, BPF_MAP_TYPE_HASH);
+    __uint(key_size, sizeof(struct bpf_sock_tuple));
+    __uint(value_size, sizeof(struct match_ctx));
+    __uint(max_entries, 256);


Is 256 records sufficient in a large number of concurrent requests?

supercharge-xsy · 2024-10-12T08:12:34Z

bpf/kmesh/workload/xdp.c

+
+    // If auth denied, it still returns XDP_PASS here, so next time when a client package is
+    // sent to server, it will be shutdown since server's RST has been set
+    return XDP_PASS;


It doesn't go here. It doesn't correspond to the comments.

supercharge-xsy · 2024-10-12T08:15:08Z

bpf/kmesh/workload/xdp.c

@@ -159,18 +111,18 @@ static inline int match_workload_policy(struct xdp_info *info, struct bpf_sock_t
            if (!policy) {
                continue;
            }
-            if (do_auth(policy, info, tuple_info) == AUTH_DENY) {
-                BPF_LOG(ERR, AUTH, "policy %u manage result deny\n", policyId);
+            if (do_auth(ctx, policy) == AUTH_DENY) {


The tail_call mechanism is used. There is no return value.

supercharge-xsy · 2024-10-12T08:23:17Z

bpf/kmesh/workload/include/authz.h

@@ -194,7 +317,7 @@ do_auth(Istio__Security__Authorization *policy, struct xdp_info *info, struct bp
        if (!rule) {
            continue;
        }
-        if (rule_match_check(rule, info, tuple_info) == MATCHED) {
+        if (rule_match_check(ctx, rule, policy->action) == MATCHED) {


Match should not be returned. If the processing is successful in tail_call, will use taill_call result. If cant handle (the configuration is abnormal), bypass the socket directly.

supercharge-xsy · 2024-10-12T08:38:04Z

bpf/kmesh/workload/include/authz.h

+        BPF_LOG(ERR, AUTH, "Failed to get tuple key\n");
+        return XDP_ABORTED;
+    }
+    res.match = match;


Suggested change

res.match = match;

res.match = match_info;

supercharge-xsy · 2024-10-12T08:40:14Z

bpf/kmesh/workload/include/authz.h

+    struct xdp_info info = {0};
+    int ret;
+
+    if (get_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {


Suggested change

if (get_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {

if (construct_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {

supercharge-xsy · 2024-10-12T08:41:39Z

bpf/kmesh/workload/include/authz.h

+        BPF_LOG(ERR, AUTH, "Failed to update map, error: %d\n", ret);
+        return XDP_DROP;
+    }
+    bpf_tail_call(ctx, &xdp_tailcall_map, TAIL_CALL_PORT_MATCH);


if tail_call failed, delete the tail_call_info map record

supercharge-xsy · 2024-10-12T08:48:31Z

bpf/kmesh/workload/include/authz.h


    if (match->n_destination_ports == 0 && match->n_not_destination_ports == 0) {
-        return MATCHED;
+        bpf_tail_call(ctx, &xdp_tailcall_map, TAIL_CALL_AUTH_IN_USER_SPACE);
+        return (res->action == AUTH_DENY) ? XDP_PASS : XDP_DROP;


need delete the map record if tail_call failed, and other tail_call code

supercharge-xsy · 2024-10-12T08:50:12Z

bpf/kmesh/workload/include/authz.h

    }

    if (match->n_not_destination_ports != 0) {
        notPorts = kmesh_get_ptr_val(match->not_destination_ports);
        if (!notPorts) {
-            return UNMATCHED;
+            return (res->action == AUTH_DENY) ? XDP_PASS : XDP_DROP;


If the configuration is abnormal, it is more reasonable to directly bypass the service.

YaoZengzeng · 2024-10-17T06:15:12Z

bpf/kmesh/workload/xdp.c

+
+    if (parser_xdp_info(ctx, &info) == PARSER_FAILED)
+        return XDP_PASS;
+    if (info.iph->version != 4 && info.iph->version != 6)


I think the same judgment logic is already included in parser_xdp_info?

YaoZengzeng · 2024-10-17T06:20:40Z

bpf/kmesh/workload/include/authz.h

    }

    ports = kmesh_get_ptr_val(match->destination_ports);
    if (!ports) {
-        return UNMATCHED;
+        return (res->action == AUTH_DENY) ? XDP_PASS : XDP_DROP;


Suggested change

return (res->action == AUTH_DENY) ? XDP_PASS : XDP_DROP;

return (res->action == AUTH_DENY) ? XDP_DROP : XDP_PASS;

?

YaoZengzeng · 2024-10-17T06:21:54Z

bpf/kmesh/workload/include/authz.h

+    if ((void *)(info->ethh + 1) > end)
+        return PARSER_FAILED;
+
+    // ip4|ip6 header


Suggested change

// ip4|ip6 header

// ipv4|ipv6 header

kmesh-bot requested review from hzxuzhonghu, supercharge-xsy and YaoZengzeng September 19, 2024 07:36

kmesh-bot added the size/L label Sep 19, 2024

weli-l force-pushed the dev/xdp_auth_2 branch 2 times, most recently from 74e8ab6 to ddcae11 Compare September 19, 2024 07:44

LiZhenCheng9527 reviewed Sep 19, 2024

View reviewed changes

weli-l force-pushed the dev/xdp_auth_2 branch from ddcae11 to 0561ae6 Compare September 19, 2024 08:17

supercharge-xsy reviewed Sep 20, 2024

View reviewed changes

hzxuzhonghu added the do-not-merge/hold label Sep 20, 2024

weli-l force-pushed the dev/xdp_auth_2 branch 2 times, most recently from c8d1d54 to 4fc783d Compare October 8, 2024 12:45

hzxuzhonghu removed the do-not-merge/hold label Oct 8, 2024

supercharge-xsy reviewed Oct 9, 2024

View reviewed changes

weli-l added 3 commits October 10, 2024 15:31

support xdp auth in tail call

01f35ec

Signed-off-by: weli-l <[email protected]>

modify map name and change map key from 0 to cpuid

991622b

Signed-off-by: weli-l <[email protected]>

fix comment

01b6a5e

Signed-off-by: weli-l <[email protected]>

weli-l force-pushed the dev/xdp_auth_2 branch 3 times, most recently from 5656126 to 56f179a Compare October 10, 2024 08:09

use bpf_sock_tuple as uni key

e89706b

Signed-off-by: weli-l <[email protected]>

weli-l force-pushed the dev/xdp_auth_2 branch from 56f179a to e89706b Compare October 10, 2024 09:11

remove useless para

ef29152

Signed-off-by: weli-l <[email protected]>

weli-l force-pushed the dev/xdp_auth_2 branch from ee872e3 to ef29152 Compare October 11, 2024 08:47

hzxuzhonghu reviewed Oct 12, 2024

View reviewed changes

fix comment

aa98335

Signed-off-by: weli-l <[email protected]>

supercharge-xsy reviewed Oct 12, 2024

View reviewed changes

YaoZengzeng reviewed Oct 17, 2024

View reviewed changes

weli-l closed this Nov 19, 2024

	int matchDstPorts(struct xdp_md *ctx)
	int match_dst_port(struct xdp_md *ctx)

	if (get_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {
	if (construct_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {

	return (res->action == AUTH_DENY) ? XDP_PASS : XDP_DROP;
	return (res->action == AUTH_DENY) ? XDP_DROP : XDP_PASS;

support xdp auth with tail call #869

support xdp auth with tail call #869

Conversation

weli-l commented Sep 19, 2024

kmesh-bot commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

hzxuzhonghu commented Sep 20, 2024

weli-l commented Sep 20, 2024

hzxuzhonghu commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supercharge-xsy commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2024 •

edited

Loading