interface's based hooks don't allow more than one function per intf per direction #338

msherif1234 · 2024-11-04T18:02:57Z

bpfman agent operator go into loop loading/unreloading the program incorrectly when more than on hook is used on the same interface for the same direction
for example this yaml will cause the issue to happen

apiVersion: bpfman.io/v1alpha1
kind: BpfApplication
metadata:
  labels:
    app: netobserv
  name: netobserv
spec:
  bytecode:
    image:
      url: quay.io/bpfman-bytecode/netobserv:latest
  globaldata:
    enable_dns_tracking: AQ==
    enable_network_events_monitoring: AQ==
    enable_rtt: AQ==
    sampling: AQAAAA==
  nodeselector: {}
  programs:
  - tcx:
      bpffunctionname: tcx_ingress_flow_parse
      direction: ingress
      interfaceselector:
        interfaces:
        - eth0
      priority: 0
    type: TCX
  - tcx:
      bpffunctionname: tcx_egress_flow_parse
      direction: egress
      interfaceselector:
        interfaces:
        - eth0
      priority: 0
    type: TCX
  - tcx:
      bpffunctionname: tcx_ingress_pca_parse
      direction: ingress
      interfaceselector:
        interfaces:
        - eth0
      priority: 0
    type: TCX
  - tcx:
      bpffunctionname: tcx_egress_pca_parse
      direction: egress
      interfaceselector:
        interfaces:
        - eth0
      priority: 0
    type: TCX
  - fentry:
      bpffunctionname: tcp_rcv_fentry
      func_name: tcp_rcv_established
    type: Fentry
  - kprobe:
      bpffunctionname: tcp_rcv_kprobe
      func_name: tcp_rcv_established
      offset: 0
      retprobe: false
    type: Kprobe
  - kprobe:
      bpffunctionname: rh_network_events_monitoring
      func_name: rh_psample_sample_packet
      offset: 0
      retprobe: false
    type: Kprobe
  - tracepoint:
      bpffunctionname: kfree_skb
      names:
      - skb/kfree_skb
    type: Tracepoint

with the fix

oc get bpfprograms 
NAME                            TYPE          STATUS            AGE
netobserv-fentry-907b363d       application   bpfmanLoaded      4m59s
netobserv-kprobe-2d90081d       application   bpfmanNotLoaded   4m50s
netobserv-kprobe-edd1a87c       application   bpfmanLoaded      4m55s
netobserv-tcx-0889f5a3          application   bpfmanLoaded      5m9s
netobserv-tcx-1043c6aa          application   bpfmanLoaded      5m2s
netobserv-tcx-3c193941          application   bpfmanLoaded      5m19s
netobserv-tcx-e8296a2e          application   bpfmanLoaded      5m6s
netobserv-tracepoint-0adc0447   application   bpfmanLoaded      4m43s

anfredette

This looks good, but I think we have the same potential problem for all program types. E.g., you couldn't attach two different kprobe programs to the same function. You might not need to do this right now, but it's a possible use case, so I think we should make the same change for the appProgramId for all types.

msherif1234 · 2024-11-04T19:17:29Z

This looks good, but I think we have the same potential problem for all program types. E.g., you couldn't attach two different kprobe programs to the same function. You might not need to do this right now, but it's a possible use case, so I think we should make the same change for the appProgramId for all types.

I don't think we can attach the same kprobe/uprobe multiple times ?

codecov · 2024-11-04T20:07:55Z

Codecov Report

Attention: Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.

Project coverage is 27.64%. Comparing base (68f13ae) to head (55cd2da).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
controllers/bpfman-agent/application-program.go	18.18%	9 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #338   +/-   ##
=======================================
  Coverage   27.64%   27.64%           
=======================================
  Files          87       87           
  Lines        7499     7499           
=======================================
  Hits         2073     2073           
  Misses       5226     5226           
  Partials      200      200

Flag	Coverage Δ
unittests	`27.64% <18.18%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anfredette · 2024-11-04T20:37:40Z

controllers/bpfman-agent/application-program.go

@@ -78,7 +78,7 @@ func (r *BpfApplicationReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		for j, p := range a.Spec.Programs {
 			switch p.Type {
 			case bpfmaniov1alpha1.ProgTypeFentry:
-				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), sanitize(p.Fentry.FunctionName))
+				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), p.Fentry.BpfFunctionName)


I don't think we can make this same change to the types that don't support a list of attach points. Or, if we do, we should still include the attach point itself, because if a user wants to attach the same bpf program to multiple Fentry attach points (for example), they'll need to use multiple Fentry program entries.

Thinking through this more, I think we need both the attach point and the bpf program name.

We can get away without the attach point for the programs that support multiple attach points by mandating that people use a single entry in the bpfApplication CRD for a given program-type/bpf-program-name instead of using multiple entries of the same program type for different attach points.

anfredette · 2024-11-04T20:39:03Z

controllers/bpfman-agent/application-program.go

@@ -100,7 +100,7 @@ func (r *BpfApplicationReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 				complete, res, err = r.reconcileCommon(ctx, rec, fentryObjects)

 			case bpfmaniov1alpha1.ProgTypeFexit:
-				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), sanitize(p.Fexit.FunctionName))
+				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), p.Fexit.BpfFunctionName)


See comment on Fentry.

anfredette · 2024-11-04T20:39:17Z

controllers/bpfman-agent/application-program.go

@@ -123,7 +123,7 @@ func (r *BpfApplicationReconciler) Reconcile(ctx context.Context, req ctrl.Reque

 			case bpfmaniov1alpha1.ProgTypeKprobe,
 				bpfmaniov1alpha1.ProgTypeKretprobe:
-				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), sanitize(p.Kprobe.FunctionName))
+				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), p.Kprobe.BpfFunctionName)


See comment on Fentry.

anfredette · 2024-11-04T20:39:26Z

controllers/bpfman-agent/application-program.go

@@ -146,7 +146,7 @@ func (r *BpfApplicationReconciler) Reconcile(ctx context.Context, req ctrl.Reque

 			case bpfmaniov1alpha1.ProgTypeUprobe,
 				bpfmaniov1alpha1.ProgTypeUretprobe:
-				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), sanitize(p.Uprobe.FunctionName))
+				appProgramId := fmt.Sprintf("%s-%s", strings.ToLower(string(p.Type)), p.Uprobe.BpfFunctionName)


See comment on Fentry.

…ce per direction Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>

anfredette

/lgtm

msherif1234 requested a review from anfredette November 4, 2024 18:03

msherif1234 changed the title ~~interface names hook doesn't allow more than one function per intf per direction~~ interfaces based hooks don't allow more than one function per intf per direction Nov 4, 2024

msherif1234 changed the title ~~interfaces based hooks don't allow more than one function per intf per direction~~ interface's based hooks don't allow more than one function per intf per direction Nov 4, 2024

anfredette requested changes Nov 4, 2024

View reviewed changes

msherif1234 force-pushed the dbg_noo branch 2 times, most recently from a9ef581 to b0eb74b Compare November 4, 2024 20:01

msherif1234 requested a review from anfredette November 4, 2024 20:02

anfredette requested changes Nov 4, 2024

View reviewed changes

interface names hook doesn't allow more than one function per interfa…

55cd2da

…ce per direction Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>

msherif1234 force-pushed the dbg_noo branch from b0eb74b to 55cd2da Compare November 4, 2024 21:16

msherif1234 requested a review from anfredette November 4, 2024 21:17

anfredette approved these changes Nov 4, 2024

View reviewed changes

mergify bot merged commit d86dfcd into bpfman:main Nov 4, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interface's based hooks don't allow more than one function per intf per direction #338

interface's based hooks don't allow more than one function per intf per direction #338

msherif1234 commented Nov 4, 2024

anfredette left a comment

msherif1234 commented Nov 4, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

anfredette Nov 4, 2024

anfredette Nov 4, 2024

anfredette Nov 4, 2024

anfredette Nov 4, 2024

anfredette Nov 4, 2024

anfredette Nov 4, 2024

anfredette left a comment

interface's based hooks don't allow more than one function per intf per direction #338

interface's based hooks don't allow more than one function per intf per direction #338

Conversation

msherif1234 commented Nov 4, 2024

anfredette left a comment

Choose a reason for hiding this comment

msherif1234 commented Nov 4, 2024

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

anfredette Nov 4, 2024

Choose a reason for hiding this comment

anfredette Nov 4, 2024

Choose a reason for hiding this comment

anfredette Nov 4, 2024

Choose a reason for hiding this comment

anfredette Nov 4, 2024

Choose a reason for hiding this comment

anfredette Nov 4, 2024

Choose a reason for hiding this comment

anfredette Nov 4, 2024

Choose a reason for hiding this comment

anfredette left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 4, 2024 •

edited

Loading