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