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.

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

new: fsconfig support #606

Merged
merged 19 commits into from
Sep 19, 2022
Merged

new: fsconfig support #606

merged 19 commits into from
Sep 19, 2022

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Sep 14, 2022

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area libscap
/area tests

Does this PR require a change in the driver versions?

/version driver-SCHEMA-version-minor

That was already bumped since last release.

What this PR does / why we need it:

See #605

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

In both 3 drivers implementation, it was chosen to return empty params for user-provided pointers in case of failure, to avoid reading out of bounds data.
Moreover, we chose to split up 4th param in 2:

  • value_bytebuf
  • value_charbuf

because it has different meaning in different situation (see https://elixir.bootlin.com/linux/latest/source/fs/fsopen.c#L314).

There is no need for any userspace parser, because at the moment we haven't got support for filesystem related FD flags/informations.

Does this PR introduce a user-facing change?:

new: add support for fsconfig syscall

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 14, 2022

/milestone 3.0.0+driver

@poiana poiana added this to the 3.0.0+driver milestone Sep 14, 2022
driver/ppm_fillers.h Outdated Show resolved Hide resolved
driver/ppm_flag_helpers.h Show resolved Hide resolved
driver/ppm_flag_helpers.h Show resolved Hide resolved
driver/event_table.c Outdated Show resolved Hide resolved
@jasondellaluce
Copy link
Contributor

I also wonder if this event is (or should be) compatible with extraction fields such as: evt.buffer, evt.buflen, evt.is_io, evt.res, fd.*

@FedeDP FedeDP changed the title new: fsconfig support wip: new: fsconfig support Sep 14, 2022
@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 15, 2022

@jasondellaluce
evt.is_io and evt.res filterchecks work out of the box.
I am currently investigating evt.buf* ones ;)
Regarding fd ones, i am not sure we are able to extract anything considering that we are not yet catching fsopen or fspick syscalls, that actually generate the FileSystem FD.

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 15, 2022

So, to make it work with evt.buffer we'd need to rename value_bytebuf param to data, or add some custom logic:

const char* resolved_argstr;
const char* argstr;
argstr = evt->get_param_value_str("data", &resolved_argstr, m_inspector->get_buffer_format());
*len = evt->m_rawbuf_str_len;
return (uint8_t*)argstr;

TBH i don't think we really need it, because one can just use evt.arg[3] at that point.
Moreover, buflen instead needs an fdinfo:

if(evt->m_fdinfo && evt->get_category() & EC_IO_BASE)
{
	return extract_buflen(evt, len);
}

that we won't have, as said before, until we gain support fsopen or fspick.

@FedeDP
Copy link
Contributor Author

FedeDP commented Sep 15, 2022

Note: i'd love to push for fspick and fsopen syscalls too, but given the time constraints for the release, i'd avoid adding more new low-level driver code now.

@FedeDP FedeDP changed the title wip: new: fsconfig support new: fsconfig support Sep 15, 2022
@jasondellaluce
Copy link
Contributor

TBH i don't think we really need it, because one can just use evt.arg[3] at that point.
Sounds good then!

Andreagit97 and others added 4 commits September 19, 2022 16:48
Co-authored-by: Andrea Terzolo <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
…le flags.

Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Jason Dellaluce <[email protected]>
leogr
leogr previously approved these changes Sep 19, 2022
@poiana poiana added the lgtm label Sep 19, 2022
@poiana
Copy link
Contributor

poiana commented Sep 19, 2022

LGTM label has been added.

Git tree hash: 95f68d6de10c3c1df0474a81f28b33a0193e6f70

LucaGuerra
LucaGuerra previously approved these changes Sep 19, 2022
Copy link
Contributor

@LucaGuerra LucaGuerra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@FedeDP FedeDP dismissed stale reviews from LucaGuerra and leogr via 6e83ce9 September 19, 2022 15:16
@poiana poiana removed the lgtm label Sep 19, 2022
@poiana poiana added the lgtm label Sep 19, 2022
@poiana
Copy link
Contributor

poiana commented Sep 19, 2022

LGTM label has been added.

Git tree hash: 72c1606334d100fe50ce89a973f3d12bcc99963f

@poiana
Copy link
Contributor

poiana commented Sep 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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,LucaGuerra,leogr]

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

@poiana poiana merged commit d60a620 into master Sep 19, 2022
@poiana poiana deleted the new/fsconfig branch September 19, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants