Support major/minor device numbers for fd events [SMAGENT-1316] #1315

gnosek · 2019-02-11T17:57:01Z

Add fd.dev, fd.dev.major and fd.dev.minor filters to enable filtering by underlying device, or e.g. just limit the events reported to physical disks (fd.dev.major > 0)

mattpag · 2019-02-12T09:52:21Z

userspace/libsinsp/parsers.cpp

 		ASSERT(parinfo->m_len == sizeof(uint32_t));
 		flags = *(uint32_t *)parinfo->m_val;

+		parinfo = evt->get_param(4);


Please take a look at https://github.com/draios/sysdig/wiki/Modify-a-pre-existent-event-or-block-entry and patch the changes to this function to safely access event parameters that may be not available when reading older captures.

I'd like to add the device field to scap files as well. Am I correct that this will require introducing SCAP_FILE_V3?

No, as for updates of pre-existent events, changing the block entry version is no longer required. In the same document I've linked to you above there's a section describing the high level changes that you'll need to make.
To test the result, you can create captures with and without the new field and verify (with a debug-enabled sysdig build) that you're able to read them with both the new and the old code.

speedyguy17 · 2019-02-15T21:44:38Z

userspace/libsinsp/logger.cpp

 #include "sinsp.h"
 #include "sinsp_int.h"

+const size_t ENCODE_LEN = 6;


SEVERITY_ENCODE_LEN ?

this is @bertocci 's patch I picked up as a part of the PR (some logging fixes for mountedfs_reader)

would it make sense to have a separate review for that since it seems several of the comments pertained to that?

speedyguy17 · 2019-02-15T21:48:27Z

driver/bpf/filler_helpers.h

 {
-	struct file *file;
-	struct dentry *dentry;
+	unsigned major = MAJOR(dev);


presumably the definition of this function MAJOR matches the below get_device_major?

Nope, the get_device_*() were wrong. Fixed.

speedyguy17 · 2019-02-15T21:48:37Z

driver/bpf/filler_helpers.h

-	struct dentry *dentry;
+	unsigned major = MAJOR(dev);
+	unsigned minor = MINOR(dev);
+	return (minor & 0xff) | (major << 8) | ((minor & ~0xff) << 12);


who picks up this encoding?

MAJOR and MINOR revert the encoding done by makedev, this is all part of the ABI (sys/types.h, st_dev encoding in struct stat). The kernel stores the device as a single 32-bit number which we decode (in various places) to a major:minor pair.

speedyguy17 · 2019-02-15T21:50:55Z

userspace/libsinsp/logger.cpp

 {
-	if(log_output_type & (sinsp_logger::OT_STDOUT | sinsp_logger::OT_STDERR)) 
+	// Store these values and clear them so we can do other bit tests
+	bool include_ts = (log_output_type & OT_NOTS) == 0;


kind of weird to do a numerical comparison here and not below? might be consistent to just do !(log_output_type & OT_NOTS)

why not just make a copy of the whole log_output_type and perform the tests against that?

cc @bertocci

speedyguy17 · 2019-02-15T22:03:44Z

userspace/libsinsp/logger.cpp

 		time_t rawtime = (time_t)ts.tv_sec;
 		struct tm* time_info = gmtime(&rawtime);
-		snprintf(m_tbuf, sizeof(m_tbuf), "%.2d-%.2d %.2d:%.2d:%.2d.%.6d ",
+		snprintf(&m_tbuf[prefix_len], sizeof(m_tbuf), "%.2d-%.2d %.2d:%.2d:%.2d.%.6d ",


shouldn't this be sizeof(m_tbuf) - prefix_len?

Looks like it, @bertocci fyi.

speedyguy17 · 2019-02-15T22:05:13Z

userspace/libsinsp/logger.cpp

 			(int)ts.tv_usec);
-		msg.insert(0, m_tbuf, 22);
+		// 22 including trailing space "10-31 23:59:59.123456 "
+		prefix_len += 22;


snprintf returns the number of characters written. it would be safer to use that directly. we can assert it's as expected if necessary

speedyguy17 · 2019-02-15T22:09:45Z

userspace/libsinsp/logger.cpp

+const char *sinsp_logger::encode_severity(const severity sev)
+{
+	// All severity strings should be ENCODE_LEN chars long
+	static const std::string pre("SEV");


These should be defined along side the enum where we define the severities, and probably can be put in an array we can directly index.

speedyguy17 · 2019-02-15T22:24:16Z

userspace/libsinsp/logger.cpp

+
+	if (str.length() < ENCODE_LEN)
+	{
+		return static_cast<severity>(SEV_MAX+1);


SEV_MAX + 1 ? so not really the max? perhaps SEV_UNSPECIFIED or something?

speedyguy17 · 2019-02-15T22:26:27Z

userspace/libsinsp/logger.cpp

+
+	// Try from max->min because we expect fewer logs for crit, error, etc.
+	const std::string prefix = str.substr(0, ENCODE_LEN);
+	for (int ii = SEV_MAX; ii >= SEV_MIN; ii--)


gnosek · 2019-02-18T19:38:22Z

@mattpag , I added the code to save/restore device ids in scap files and to get them during the initial /proc scan, please take a look. I'm not a fan of how the /proc scan code turned out (esp. with the hash table), suggestions appreciated

gnosek · 2019-02-19T15:31:39Z

@mattpag , I tested recording scap files with old/new driver/sysdig combinations (except for old/old) and the reading them back with old/new versions:

fd.name contains /dev/shm (old and new sysdig)
fd.dev.major=0 and fd.dev.minor=21 (new sysdig only)

everything works fine (where possible, i.e. can't expect fd.dev.* to work on a scap file captured with old sysdig, regardless of the driver version)

Any further comments?

nathan-b · 2019-02-20T14:13:54Z

driver/bpf/fillers.h

 	mode = open_modes_to_scap(val, mode);
 	res = bpf_val_to_ring(data, mode);
+	if (res != PPM_SUCCESS)
+		return res;


We have single-line conditionals without braces {} in the same function as single-line conditionals with braces (line 168). Please just include braces around all your conditionals.

nathan-b · 2019-02-20T14:15:57Z

driver/bpf/fillers.h

+	{
+		dev = 0;
+	}
+	res = bpf_val_to_ring(data, dev);


It seems like you're overwriting the value of res assigned on line 2216 without checking it. Please check res against PPM_SUCCESS at the end of the mode block.

good catch!

nathan-b · 2019-02-20T14:17:00Z

driver/bpf/fillers.h

+	 * Device
+	 */
+	if (retval < 0 || !bpf_get_fd_dev_ino(retval, &dev, &ino))
+	{


Again, here we have braces around a single-line conditional, while above we don't. Please use braces around all conditionals for consistency and safety.

(I understand that some of this code was C&P, but at least with this change we can avoid making the problem worse ;)

Please keep in mind this is kernel code, and so it should respect the kernel coding conventions (as specified on the top of https://github.com/draios/sysdig/blob/dev/coding_conventions.md). Kernel coding conventions use no braces for single statements block. For all this code I've tried to respect them. You can use the convenient and thorough checkpatch.pl script in the kernel sources, which will suggest you the right formatting changes for a given file (some suggestions are non actionable for external code like ours, but most are).

I can also see a few formatting errors slipped in with the last batch of changes when getsockopt support was implemented in this code.

good point about checkpatch.pl, I forgot about it. Now the PR is checkpatch-clean except for some long lines. I'll clean up getsockopt with a separate PR.

userspace/libscap/scap_fds.c

The encoding is compatible with st_dev in (userspace) struct stat

* fd.dev=DEVICE (st_dev compatible layout) * fd.dev.major=MAJOR (major device number, decimal) * fd.dev.minor=MINOR (minor device number, decimal)

gnosek requested review from gianlucaborello, mattpag and nathan-b February 11, 2019 17:57

mattpag requested changes Feb 12, 2019

View reviewed changes

anoop-sysd changed the title ~~Support major/minor device numbers for fd events~~ Support major/minor device numbers for fd events [SMAGENT-1319] Feb 12, 2019

gnosek force-pushed the nfs branch from 6a21de1 to c1fd43d Compare February 14, 2019 14:57

speedyguy17 reviewed Feb 15, 2019

View reviewed changes

gnosek force-pushed the nfs branch from c1fd43d to 762ac38 Compare February 18, 2019 15:03

gnosek force-pushed the nfs branch 2 times, most recently from dfe8832 to a5ea3cc Compare February 19, 2019 15:27

nathan-b suggested changes Feb 20, 2019

View reviewed changes

gnosek commented Feb 25, 2019

View reviewed changes

userspace/libscap/scap_fds.c Show resolved Hide resolved

gnosek force-pushed the nfs branch from a5ea3cc to 57885c6 Compare February 25, 2019 16:54

gnosek force-pushed the nfs branch 3 times, most recently from e399a35 to 0fb14de Compare March 7, 2019 16:55

gnosek force-pushed the nfs branch from 0fb14de to 054926a Compare March 29, 2019 16:14

gnosek force-pushed the nfs branch 2 times, most recently from c09da10 to e5c8f56 Compare April 12, 2019 14:38

gnosek force-pushed the nfs branch from e5c8f56 to c0b5ff3 Compare April 17, 2019 15:05

gnosek requested review from mattpag and nathan-b April 17, 2019 15:11

gnosek changed the title ~~Support major/minor device numbers for fd events [SMAGENT-1319]~~ Support major/minor device numbers for fd events [SMAGENT-1316] Apr 17, 2019

gnosek added 5 commits April 29, 2019 15:27

[eBPF] Get device number along with inode

a297e58

Get device number for open/openat/creat events

6380600

The encoding is compatible with st_dev in (userspace) struct stat

Add filters to match fd device numbers

681411b

* fd.dev=DEVICE (st_dev compatible layout) * fd.dev.major=MAJOR (major device number, decimal) * fd.dev.minor=MINOR (minor device number, decimal)

Get file device ids during initial /proc scan

a161c9f

Save/restore file device_id in scap files

6529278

gnosek force-pushed the nfs branch from c0b5ff3 to 6529278 Compare April 29, 2019 13:28

gnosek merged commit 9ef2d64 into dev Apr 29, 2019

gnosek deleted the nfs branch April 29, 2019 14:07

Support major/minor device numbers for fd events [SMAGENT-1316] #1315

Support major/minor device numbers for fd events [SMAGENT-1316] #1315

Uh oh!

Conversation

gnosek commented Feb 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnosek commented Feb 18, 2019

Uh oh!

gnosek commented Feb 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants