Add FileOp specific ShouldHandleFileOpEvent helper to kext

[wilbaker]

Cleanup work for #152 (related to change in #156).

We should have separate logic for deciding if FileOp and VnodeOp events should be handled. In #156 we incorrectly used the same helper function for both.

[sanoursa]

A lot of the code in this function duplicates the code in ShouldHandleVnodeOpEvent. Does it make sense to refactor them to call a common method(s) for the shared logic?

[wilbaker]

Did you have some specific helper(s) in mind?

I could add these:

• Move the call of vnode_vtype into ShouldIgnoreVnodeType (and provide the vtype as an output parameter)
• Add an inline IsInVirtualizationRoot helper with a vnodeFileFlags output parameter
• Add an inline HasRegisteredProvider that just checks returns nullptr == (*root)->providerUserClient

But the code for those is already pretty small, and so I'm on the fence as to whether helpers add much value.

Thoughts?

[sanoursa]

It just felt like a lot of duplication, though I agree that breaking it down into a bunch of small helpers is probably overkill.

It appears like the vnode function is a superset of the fileop function. Could this just be broken into two methods, one of which calls the other? What I'd like to avoid is duplicated efforts in bug fixing, adding more common checks, etc over time.

[wilbaker]

It appears like the vnode function is a superset of the fileop function. Could this just be broken into two methods, one of which calls the other?

I think we could have ShouldHandleFileOpEvent call ShouldHandleVnodeOpEvent and carefully pass in dummy values for action to avoid having it go down paths we don't want it to.

There are some downsides to that approach though:

• kext would be performing extra work in a performance critical path
• Potentially more fragile as changes to ShouldHandleVnodeOpEvent could inadvertently break ShouldHandleFileOpEvent.

For long term maintainability I like the common helper methods approach as it helps keep clear exactly how we're making the "ShouldHandle" decision for each type of event.

What I'd like to avoid is duplicated efforts in bug fixing, adding more common checks, etc over time.

I completely agree on this goal. What would you think if we continue to monitor these functions as we implement the missing callbacks\notifications, and revisit this before considering #152 complete?

As I've started working on the other notifications (in parallel with this PR) I can already see how ShouldHandleFileOpEvent will need additional changes that might actually break the subset\superset relationship with ShouldHandleVnodeOpEvent.

If it's OK with you I think it would be cleaner to iterate on these functions separately and then decide the best way to share code once we have the basic set of notifications working.

[sanoursa]

That makes sense to me 👍

[sanoursa]

Why is there no procname here? Don't we still want to include that in any messages to the provider?

[wilbaker]

If ShouldHandleFileOpEvent returns true, HandleFileOpOperation will look up the proc name using the pid output parameter.

Unlike ShouldHandleVnodeOpEvent this function does not need to use procname in its decision making process. It didn't seem right to me to leave it as an output parameter when it's unrelated to the work the function is doing (deciding if the FileOp should be handled).

[sanoursa]

Just including the action value is not all that useful, IMO. Maybe create a helper function for splitting out the action bits like we do for vnodes?

[wilbaker]

Do you mean that it would be more useful to see the name (e.g. KAUTH_FILEOP_CLOSE) than the value?

Unlike the vnode events the action in file op events is not a bitset:

/* Actions */
#define KAUTH_FILEOP_OPEN			1
#define KAUTH_FILEOP_CLOSE			2
#define KAUTH_FILEOP_RENAME			3
#define KAUTH_FILEOP_EXCHANGE	        	4
#define KAUTH_FILEOP_LINK			5
#define KAUTH_FILEOP_EXEC			6
#define KAUTH_FILEOP_DELETE			7

[sanoursa]

Ah yea it's a lot simpler to interpret in that case then. We can always add a string translation later on if needed for debugging.