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
