Re: [PATCH 09/17] mm: export access_remote_vm() symbol

[Lorenzo Stoakes <lstoakes@gmail.com>]

On Tue, Jan 03, 2023 at 07:20:11PM +0000, Yu, Fenghua wrote:
> Hi, Lorenzo,
>

Hey Fenghua :)

> access_remote_vm(mm) directly call __access_remote_vm(mm).
> access_process_vm(tsk) calls mm=get_task_mm() then __access_remote_vm(mm).
>
> So instead of access_remote_vm(mm), it's access_process_vm(tsk) that holds
> a reference count on the mm, right?

Indeed!

>
> > >
> > > Is there a reason you can't use access_process_vm() which is exported
> > > and additionally handles the refrencing?
>
> IDXD interrupt handler starts a work which needs to access remote vm.
> The remote mm is found by PASID which is saved in device event log.
>
> In the work, it's hard to get the remote mm from a task because mm->owner could be NULL
> but the mm is still existing.

That makes sense, however I do feel nervous about exporting something that that
relies on this reference.

The issue is ensuring that the mm can't be taken from underneath you, the only
user of access_remote_vm(), procfs, does a careful dance using get_task_mm() and
mm_access() to ensure this can't happen, if _sometimes_ the remote mm might have
an owner and _sometimes_ not it feels like any exported function needs to be
equally careful?

I definitely don't feel as if simply exporting this is a safe option, and you
would in that case need a new function that handles different scenarios of mm
ownership/not.

I may be missing something here and I will wait for others to chime in but I
think we would definitely need something more than simply exporting this.