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

docs(userspace/libsinsp): more detailed docs about proc filterchecks #1095

Merged
merged 1 commit into from
May 17, 2023

Conversation

therealbobo
Copy link
Contributor

What type of PR is this?

/kind documentation

Any specific area of the project related to this PR?

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

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

This clarifies in the docs the behaviour of some filters of the processes.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@Andreagit97 Andreagit97 added this to the 0.11.0 milestone May 15, 2023
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Overall, SGTM, thank you!

Just left a couple of comments

userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.pname", "Parent Name", "The name (excluding the path) of the parent of the process generating the event."},
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.aname", "Ancestor Name", "The name (excluding the path) of one of the process ancestors. e.g. proc.aname[1] returns the parent name, proc.aname[2] returns the grandparent name, and so on. proc.aname[0] is the name of the current process. proc.aname without arguments can be used in filters only and matches any of the process ancestors, e.g. proc.aname=bash."},
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.args", "Arguments", "The arguments passed on the command line when starting the process generating the event excluding argv[0]."},
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.exe", "First Argument", "The first command line argument argv[0] (usually the executable name or a custom one). This field is updated on a procfs scan (in this case the field is read from /proc/<pid>/cmdline) or when a exceve or close syscall occurs (in this case the field is read from the syscall args)."},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.exe", "First Argument", "The first command line argument argv[0] (usually the executable name or a custom one). This field is updated on a procfs scan (in this case the field is read from /proc/<pid>/cmdline) or when a exceve or close syscall occurs (in this case the field is read from the syscall args)."},
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.exe", "First Argument", "The first command line argument argv[0] which is usually the executable name or a custom string. This field is initially updated through a procfs scan (specifically by reading from /proc/<pid>/cmdline), then through the syscall events stream at runtime (in this case, the field is read from the syscall args)."},

I'd not list the exact syscalls since they can change over time. I'd just mention

  • procfs scan (only at starting time, pls this needs to be confirmed)
  • syscall events stream (at runtime, without mentioning all the syscalls.

(Pls note, my suggestion is just a proposal, not necessarily the best way to document that field)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really like these suggestions! Also I took a look at the code and the procfs scan is done at starting time only as you said! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took another look and, to be precise, the Procfs scans could also happen in live mode. e.g.:

evt->m_tinfo = m_inspector->get_thread_ref(evt->m_pevt->tid, query_os, false).get();

Given that I think that the best approach is to be more generic: documenting it in a very precise way could be a problem and lead to inconsistencies for the final user if the behaviour changes.

userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.exe", "First Argument", "The first command line argument argv[0] which is usually the executable name but it could be also a custom string, it depends on what the user specifies. This field could be populated through a procfs scan (specifically by reading from /proc/<pid>/cmdline) or through the syscall events stream at runtime (in this case, the field is read from the syscalls args)."},
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.pexe", "Parent First Argument", "The proc.exe (first command line argument argv[0]) of the parent process."},
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.aexe", "Ancestor First Argument", "The proc.exe (first command line argument argv[0]) of one of the process ancestors. e.g. proc.aexe[1] returns the parent first argument, proc.aexe[2] returns the grandparent first argument, and so on. proc.aexe[0] is the first argument of the current process. proc.aexe without arguments can be used in filters only and matches any of the process ancestors, e.g. proc.aexe endswith java."},
{PT_CHARBUF, EPF_NONE, PF_NA, "proc.exepath", "Process Executable Path", "The full executable path of the process. This field could be populated through a procfs scan (in this case the field is the resolved path of /proc/<pid>/exe) or through the syscall events stream at runtime (in this case, the field is read from the syscalls args)."},
Copy link
Member

Choose a reason for hiding this comment

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

same as above for at runtime

Copy link
Contributor

Choose a reason for hiding this comment

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

re exepath, should we be very clear that this applies only to executables that have an image on disk / on the filesystem? Since we are working on memfd+exec flag ...

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Overall, SGTM. However, after a second look, I believe we should be more transparent regarding all those implications that are not self-evident. See my comments below.

userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
@therealbobo therealbobo force-pushed the docs/new-proc branch 3 times, most recently from 94669f4 to 347bf28 Compare May 17, 2023 16:04
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
userspace/libsinsp/filterchecks.cpp Outdated Show resolved Hide resolved
Signed-off-by: Roberto Scolaro <roberto.scolaro21@gmail.com>
Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented May 17, 2023

LGTM label has been added.

Git tree hash: f4e5834bb9db1775047bd4582806ee11bccf666d

Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

Thanks a bunch @therealbobo for your help🙏 !

/approve

@poiana
Copy link
Contributor

poiana commented May 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, incertum, therealbobo

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 [Andreagit97,incertum]

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 5e30689 into falcosecurity:master May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants