API versioning for user/kernel boundary #39

gnosek · 2021-05-07T17:14:12Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area driver-kmod

/area driver-ebpf

/area libscap

/area libsinsp

/area tests

What this PR does / why we need it:

This PR implements the API versioning proposal.

It basically:

adds an API version number (encoded in a 64-bit int) included in the userspace component and both kmod and eBPF probes
replaces the eBPF probe_version equality check with a semver(like) check on the contents of the probe_api_version section; this allows us to reuse eBPF probes across consumer/libs versions as long as they support the same API
adds a version check to the kmod open path in scap_open_live_int(); there was no such check before
adds driver/README.API_VERSION documenting the API versioning rules

It also takes care to reject probes predating API versioning.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The kernel module version (as seen in e.g. `modinfo`) no longer defaults to FALCOSECURITY_LIBS_VERSION (which is generally a commit id) but to the API version (1.0.0 as of now)

action required: ensure any infrastructure for building probes uses the API version as the identifier, not the consumer version

action not required but recommended: don't override PROBE_NAME or PROBE_VERSION when building falco-libs. Your kernel module will now be named `scap.ko` with version 1.0.0 and can be shared with all other falco-libs consumers using the same kernel.

poiana · 2021-05-07T17:14:13Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

poiana · 2021-08-08T20:09:21Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

poiana · 2021-09-08T14:17:37Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh with /remove-lifecycle rotten.

Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle rotten

poiana · 2021-10-30T14:47:58Z

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

poiana · 2021-10-30T14:48:01Z

@poiana: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue with /reopen.

Mark the issue as fresh with /remove-lifecycle rotten.

Provide feedback via https://github.com/falcosecurity/community.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

leogr · 2021-10-30T17:35:36Z

/reopen
/remove-lifecycle rotten

poiana · 2021-10-30T17:35:39Z

@leogr: Reopened this PR.

In response to this:

/reopen
/remove-lifecycle rotten

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

userspace/libscap/scap.c

leogr

Just some thoughts about naming (see my comments below).

I also have the same question @FedeDP wrote in this comment 👇
#39 (review)

Otherwise, SGTM 👏

userspace/libscap/scap.c

gnosek · 2022-02-02T13:44:47Z

@leogr, honestly I'd rather not introduce the new naming here, even in new APIs. I'm all for a subsequent PR that renames everything at once that we merge soon after (nobody should start depending on the *probe* functions/methods in the meantime and we won't end up with a halfway-done rename)

I'm open to discussion if you have strong opinions about it though

leogr · 2022-02-02T13:57:46Z

@leogr, honestly I'd rather not introduce the new naming here, even in new APIs. I'm all for a subsequent PR that renames everything at once that we merge soon after (nobody should start depending on the *probe* functions/methods in the meantime and we won't end up with a halfway-done rename)

I'm open to discussion if you have strong opinions about it though

👍 agreed. If you want, I can make the PR for the renaming once this gets merged.

FedeDP

LGTM!
/approve

poiana · 2022-02-02T14:14:11Z

LGTM label has been added.

Git tree hash: 34e54e57f0ecd42927fea2ff9a641ca9e37deb21

This allows reusing eBPF probes across different consumers (and consumer versions) as long as the API is compatible. It also adds validation of API versions in the non-eBPF kernel probes, which was a long-standing omission. Since the actual interface evolved slowly, it generally worked fine, but in some rare cases it could easily end up with a kernel panic. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

If fcntl(F_SETFD, FD_CLOEXEC) failed in scap_open_live_int, we returned from the function with the fd still open (scap_close only closes the fds which have a mmapped buffer attached). Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

leogr

/approve

This feature is a huge step forward for this project.
@gnosek huge thank you! 🙏

poiana · 2022-02-02T16:47:06Z

LGTM label has been added.

Git tree hash: 45a54ab12ef309a57c2e89db359be5e2fead9168

poiana · 2022-02-02T16:47:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, gnosek, leogr

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,gnosek,leogr]

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

gnosek requested review from leodido, ldegio, fntlnz and leogr May 7, 2021 17:14

poiana added do-not-merge/work-in-progress area/build dco-signoff: no area/driver-kmod do-not-merge/release-note-label-needed area/driver-bpf area/libscap size/L labels May 7, 2021

poiana added the lifecycle/stale label Aug 8, 2021

poiana added lifecycle/rotten and removed lifecycle/stale labels Sep 8, 2021

gnosek force-pushed the api-semver branch from 68c0a0a to 458ca19 Compare September 29, 2021 18:00

poiana added the approved label Sep 29, 2021

gnosek force-pushed the api-semver branch 2 times, most recently from d1e0f1e to 7a1b461 Compare September 30, 2021 13:45

poiana added dco-signoff: yes and removed dco-signoff: no labels Sep 30, 2021

poiana closed this Oct 30, 2021

poiana reopened this Oct 30, 2021

gnosek force-pushed the api-semver branch from 9da73e3 to 33c0632 Compare January 28, 2022 18:26

FedeDP requested changes Feb 2, 2022

View reviewed changes

userspace/libscap/scap.c Show resolved Hide resolved

poiana assigned FedeDP Feb 2, 2022

leogr reviewed Feb 2, 2022

View reviewed changes

userspace/libscap/scap.c Show resolved Hide resolved

userspace/libscap/scap.c Show resolved Hide resolved

userspace/libscap/scap.c Show resolved Hide resolved

gnosek force-pushed the api-semver branch 2 times, most recently from 4f903f7 to 22e9d39 Compare February 2, 2022 13:37

gnosek force-pushed the api-semver branch 2 times, most recently from 2ef3ea4 to 8d70834 Compare February 2, 2022 14:10

FedeDP previously approved these changes Feb 2, 2022

View reviewed changes

poiana added the lgtm label Feb 2, 2022

gnosek dismissed FedeDP’s stale review via d884600 February 2, 2022 15:53

gnosek force-pushed the api-semver branch from 8d70834 to d884600 Compare February 2, 2022 15:53

poiana removed the lgtm label Feb 2, 2022

poiana requested a review from FedeDP February 2, 2022 15:53

gnosek added 2 commits February 2, 2022 16:54

gnosek force-pushed the api-semver branch from d884600 to 1f131f1 Compare February 2, 2022 15:56

leogr approved these changes Feb 2, 2022

View reviewed changes

poiana assigned leogr Feb 2, 2022

poiana added the lgtm label Feb 2, 2022

FedeDP approved these changes Feb 2, 2022

View reviewed changes

poiana merged commit 12406fc into falcosecurity:master Feb 2, 2022

leogr mentioned this pull request Feb 3, 2022

refactor: replace probe with driver #197

Merged

gnosek mentioned this pull request Feb 14, 2022

Driver API versioning fixes #207

Merged

LucaGuerra mentioned this pull request Feb 16, 2022

fix(build): support PPM_API_CURRENT_VERSION macros falcosecurity/driverkit#125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API versioning for user/kernel boundary #39

API versioning for user/kernel boundary #39

gnosek commented May 7, 2021 •

edited

Loading

poiana commented May 7, 2021

poiana commented Aug 8, 2021

poiana commented Sep 8, 2021

poiana commented Oct 30, 2021

poiana commented Oct 30, 2021

leogr commented Oct 30, 2021

poiana commented Oct 30, 2021

leogr left a comment

gnosek commented Feb 2, 2022

leogr commented Feb 2, 2022

FedeDP left a comment

poiana commented Feb 2, 2022

leogr left a comment

poiana commented Feb 2, 2022

poiana commented Feb 2, 2022

API versioning for user/kernel boundary #39

API versioning for user/kernel boundary #39

Conversation

gnosek commented May 7, 2021 • edited Loading

poiana commented May 7, 2021

poiana commented Aug 8, 2021

poiana commented Sep 8, 2021

poiana commented Oct 30, 2021

poiana commented Oct 30, 2021

leogr commented Oct 30, 2021

poiana commented Oct 30, 2021

leogr left a comment

Choose a reason for hiding this comment

gnosek commented Feb 2, 2022

leogr commented Feb 2, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Feb 2, 2022

leogr left a comment

Choose a reason for hiding this comment

poiana commented Feb 2, 2022

poiana commented Feb 2, 2022

gnosek commented May 7, 2021 •

edited

Loading