Re: [PATCH v3 07/15] iommufd: PFN handling for iopt_pages

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux