On Tue, Nov 01, 2022 at 12:38:48PM -0700, Nicolin Chen wrote:
+static int pfn_reader_user_pin(struct pfn_reader_user *user,
struct iopt_pages *pages,unsigned long start_index,unsigned long last_index)+{
- bool remote_mm = pages->source_mm != current->mm;
- unsigned long npages;
- uintptr_t uptr;
- long rc;
- if (!user->upages) {
/* All undone in pfn_reader_destroy() */user->upages_len =(last_index - start_index + 1) * sizeof(*user->upages);user->upages = temp_kmalloc(&user->upages_len, NULL, 0);if (!user->upages)return -ENOMEM;- }
- if (user->locked == -1) {
/** The majority of usages will run the map task within the mm* providing the pages, so we can optimize into* get_user_pages_fast()*/user->locked = 0;if (remote_mm) {if (!mmget_not_zero(pages->source_mm)) {kfree(user->upages);user->upages = NULL;Coverity reports BAD_FREE at user->upages here.
In iopt_pages_fill_xarray and iopt_pages_fill_from_mm, user->upages is assigned by shifting the out_pages input of iopt_pages_add_access that could be originated from vfio_pin_pages, I am not sure if the remote_mm and mmget_not_zero value checks can prevent this though.
Yep, missed that when I reworked this. It should be like this:
* providing the pages, so we can optimize into * get_user_pages_fast() */ - user->locked = 0; if (remote_mm) { - if (!mmget_not_zero(pages->source_mm)) { - kfree(user->upages); - user->upages = NULL; + if (!mmget_not_zero(pages->source_mm)) return -EFAULT; - } } + user->locked = 0; }
ie locked is tracking the mmget and is completely independent of upages.
Jason