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>
From:   Bodo Stroesser <bostroesser@gmail.com>
Message-ID: <aa95b4db-ca88-e38c-3871-fb935f1e2212@gmail.com>
Date:   Wed, 13 Jan 2021 18:59:10 +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: <73dc2d01-6398-c1d1-df47-66034d184eec@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.