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

From: "Yu, Fenghua" <fenghua.yu@intel.com>
To: Lorenzo Stoakes <lstoakes@gmail.com>
Cc: Vinod Koul <vkoul@kernel.org>,
	"Jiang, Dave" <dave.jiang@intel.com>,
	"dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
	"Zhu, Tony" <tony.zhu@intel.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	Christoph Hellwig <hch@infradead.org>,
	"Shankar, Ravi V" <ravi.v.shankar@intel.com>
Subject: RE: [PATCH 09/17] mm: export access_remote_vm() symbol
Date: Wed, 4 Jan 2023 05:06:29 +0000	[thread overview]
Message-ID: <IA1PR11MB609745A8BE83313FAB0236C29BF59@IA1PR11MB6097.namprd11.prod.outlook.com> (raw)
In-Reply-To: <Y7SMYF8MlzeqDgp+@lucifer>

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.

I may define and export a new wrapper access_remote_vm_ref() which will hold
mm's reference count before accessing it:
int access_remote_vm_ref(mm)
{
   int ret;

   if (mm == &init_mm)
        return 0;

   mmget(mm);
   ret = access_remote_vm(mm);
   mmput(mm);

   return ret;
}
EXPORT_SYMBOL_GPL(access_remote_vm_ref);

IDXD or any driver calls this and holds mm reference count while accesses the mm.
This is useful for caller to directly access mm even if mm's owner could be NULL.

Do you think this is sufficient to take care of the mm reference and is a good way to go?

Thanks.

-Fenghua