On Mon, Oct 9, 2023 at 1:51 PM Paul Moore <p...@paul-moore.com> wrote:
> On Mon, Oct 9, 2023 at 1:41 PM John Johansen
> <john.johan...@canonical.com> wrote:
> > On 10/9/23 10:06, Paul Moore wrote:
> > > I don't think anyone is objecting to resolving this, it's more a
> > > matter of *how* we can resolve it.
> >
> > currently I am see four crazy/stupid paths forward, each with their own
> > pain points.
...
> > 4. caching a reference in the audit_context as paul has suggested.
>
> I don't like this idea, but I'm struggling to come up with something
> less awful. Below is a quick, untested patch to describe the concept
> with code. It is worth noting that we don't take a mm_struct
> reference in the io_uring entry point because I'm not sure filtering
> on the executable file makes much sense there given the async nature
> of io_uring, however I'm open to comments here (as well as pretty much
> everything else in this pseudo-patch).
Looking at this a bit more, I'm now wondering if there is a fifth
option: call mmget() directly and skip the task_lock().
Take a look at the move_pages(2) code path:
SYSCALL_DEFINE6(move_pages, ...)
-> kernel_move_pages(...)
-> find_mm_struct(...)
-> mmget(...)
In find_mm_struct(), if the task being manipulated is *not* the
current task then get_task_mm() is called, which takes task_lock().
However, if the task being manipulated *is* the current task then the
task_lock() can be avoided and a direct call to get_mm() is used;
get_mm() does a simple atomic_inc() without any additional locking.
What do you think of this approach (untested, copy-n-pasted patch):
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 65075f1e4ac8..ffd17ad97324 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -526,8 +526,19 @@ int audit_exe_compare(struct task_struct *tsk, struct audit
_fsnotify_mark *mark)
struct file *exe_file;
unsigned long ino;
dev_t dev;
+ struct mm_struct *mm;
- exe_file = get_task_exe_file(tsk);
+ /* almost always (always?) comparing @current, but handle both cases */
+ if (likely(tsk == current)) {
+ mmget(current->mm);
+ mm = current->mm;
+ } else {
+ mm = get_task_mm(tsk);
+ if (!mm)
+ return 0;
+ }
+ exe_file = get_mm_exe_file(mm);
+ mmput(mm);
if (!exe_file)
return 0;
ino = file_inode(exe_file)->i_ino;
--
paul-moore.com
