-
Notifications
You must be signed in to change notification settings - Fork 451
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
Add FileOp specific ShouldHandleFileOpEvent helper to kext #173
Add FileOp specific ShouldHandleFileOpEvent helper to kext #173
Conversation
@@ -539,6 +542,56 @@ static bool ShouldHandleEvent( | |||
return true; | |||
} | |||
|
|||
static bool ShouldHandleFileOpEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have some specific helper(s) in mind?
I could add these:
- Move the call of
vnode_vtype
intoShouldIgnoreVnodeType
(and provide thevtype
as an output parameter) - Add an inline
IsInVirtualizationRoot
helper with avnodeFileFlags
output parameter - Add an inline
HasRegisteredProvider
that just checks returnsnullptr == (*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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 breakShouldHandleFileOpEvent
.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me 👍
|
||
// Out params: | ||
VirtualizationRoot** root, | ||
int* pid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there no procname
here? Don't we still want to include that in any messages to the provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
*root = VirtualizationRoots_FindForVnode(vnode); | ||
if (nullptr == *root) | ||
{ | ||
KextLog_FileNote(vnode, "ShouldHandleFileOpEvent(%d): No virtualization root found for file with set flag.", action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
63ea8ce
to
4a4f5e7
Compare
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.