Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] scsi: target: tcmu: Fix wrong uio handling causing big
 memory leak
To:     Mike Christie <michael.christie@oracle.com>,
        linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        "Martin K. Petersen" <martin.petersen@oracle.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
References: <20201218141534.9918-1-bostroesser@gmail.com>
 <73dc2d01-6398-c1d1-df47-66034d184eec@oracle.com>
 <aa95b4db-ca88-e38c-3871-fb935f1e2212@gmail.com>
 <3caa89ba-47b8-d85c-e7a5-54d84d1471f0@oracle.com>
From:   Bodo Stroesser <bostroesser@gmail.com>
Message-ID: <f2bf0c02-0d44-4b75-7c36-5d5fb213d747@gmail.com>
Date:   Thu, 14 Jan 2021 17:50:03 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <3caa89ba-47b8-d85c-e7a5-54d84d1471f0@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 13.01.21 22:04, Mike Christie wrote:
> On 1/13/21 11:59 AM, Bodo Stroesser wrote:
>> On 12.01.21 19:36, Mike Christie wrote:
>>> On 12/18/20 8:15 AM, Bodo Stroesser wrote:
>>>> tcmu calls uio_unregister_device from tcmu_destroy_device.
>>>> After that uio will never call tcmu_release for this device.
>>>> If userspace still had the uio device open and / or mmap'ed
>>>> during uio_unregister_device, tcmu_release will not be called and
>>>> udev->kref will never go down to 0.
>>>>
>>>
>>> I didn't get why the release function is not called if you call
>>> uio_unregister_device while a device is open. Does the device_destroy call in
>>> uio_unregister_device completely free the device or does it set some bits so
>>> uio_release is not called later?
>>
>> uio_unregister_device() resets the pointer (idev->info) to the struct uio_info which tcmu provided in uio_register_device().
>> The uio device itself AFAICS is kept while it is open / mmap'ed.
>> But no matter what userspace does, uio will not call tcmu's callbacks
>> since info pointer now is NULL.
>>
>> When userspace finally closes the uio device, uio_release is called, but
>> tcmu_release can not be called.
>>
>>>
>>> Do other drivers hit this? Should uio have refcounting so uio_release is called
>>> when the last ref (from userspace open/close/mmap calls and from the kernel by
>>> drivers like target_core_user) is done?
>>>
>>
>> To be honest I don't know exactly.
>> tcmu seems to be a special case in that is has it's own mmap callback.
>> That allows us to map pages allocated by tcmu.
>> As long as userspace still holds the mapping, we should not unmap those
>> pages, because userspace then could get killed by SIGSEGV.
>> So we have to wait for userspace closing uio before we may unmap and
>> free the pages.
> 
> 
> If we removed the clearing of idev->info in uio_unregister_device, and
> then moved the idev->info->release call from uio_release to
> uio_device_release it would work like you need right? The release callback
> would always be called and called when userspace has dropped it's refs.
> You need to also fix up the module refcount and some other bits because
> it looks like uio uses the uio->info check to determine if the device is
> being removed.

I fear that would not work, because uio_release must be called always,
no matter whether userspace closes the device before or after
uio_unregister_device.

But we could add a new callback pointer 'late_release' to struct
uio_info and struct uio_device. During uio_register_device we would
copy the pointer from info to idev.

If info == NULL, uio_release calls idev->late_release if != NULL.

tcmu would of course set info->release and ->late_release both to
tcmu_release.

> 
> I don't know if that is the correct approach. It looks like non
> target_core_user drivers could hit a similar issue. It seems like drivers
> like qedi/bnx2i could hit the issue if their port is removed from the
> kernel before their uio daemon closes the device. However, they also
> could do a driver specific fix and handle the issue by adding some extra
> kernel/userspace bits to sync the port removal.
> 

I had a closer look into qedi. I assume there might be a leak also,
because qedi_uio_close calls "qedi_ll2_free_skbs(qedi)".

Unfortunately my above proposal would not work here without adding a
new refcount to qedi_uio_dev, because currently in __qedi_free_uio
the udev is freed shortly after uio_unregister_device. So later calls
of qedi_uio_close(udev) would be harmful.

But I guess the leak can be fixed by adding two lines after the
uio_unregister_device() in __qedi_free_uio:

	if (test_bit(UIO_DEV_OPENED, &udev->qedi->flags)
		qedi_ll2_free_skbs(qedi);