Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c19b3651-624b-f60e-3e63-fe9fadc6981f@gmail.com>
Date:   Tue, 18 Apr 2023 17:25:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH 5/7] io_uring: rsrc: use FOLL_SAME_FILE on
 pin_user_pages()
To:     Jason Gunthorpe <jgg@nvidia.com>,
        Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Matthew Wilcox <willy@infradead.org>,
        David Hildenbrand <david@redhat.com>,
        Jens Axboe <axboe@kernel.dk>, io-uring@vger.kernel.org
References: <cover.1681508038.git.lstoakes@gmail.com>
 <17357dec04b32593b71e4fdf3c30a346020acf98.1681508038.git.lstoakes@gmail.com>
 <ZD1CAvXee5E5456e@nvidia.com>
Content-Language: en-US
From:   Pavel Begunkov <asml.silence@gmail.com>
In-Reply-To: <ZD1CAvXee5E5456e@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/17/23 13:56, Jason Gunthorpe wrote:
> On Sat, Apr 15, 2023 at 12:27:45AM +0100, Lorenzo Stoakes wrote:
>> Commit edd478269640 ("io_uring/rsrc: disallow multi-source reg buffers")
>> prevents io_pin_pages() from pinning pages spanning multiple VMAs with
>> permitted characteristics (anon/huge), requiring that all VMAs share the
>> same vm_file.
> 
> That commmit doesn't really explain why io_uring is doing such a weird
> thing.
> 
> What exactly is the problem with mixing struct pages from different
> files and why of all the GUP users does only io_uring need to care
> about this?

Simply because it doesn't seem sane to mix and register buffers of
different "nature" as one. It's not a huge deal for currently allowed
types, e.g. mixing normal and huge anon pages, but it's rather a matter
of time before it gets extended, and then I'll certainly become a
problem. We've been asked just recently to allow registering bufs
provided mapped by some specific driver, or there might be DMA mapped
memory in the future.

Rejecting based on vmas might be too conservative, I agree and am all
for if someone can help to make it right.


> If there is no justification then lets revert that commit instead.
> 
>>   		/* don't support file backed memory */
>> -		for (i = 0; i < nr_pages; i++) {
>> -			if (vmas[i]->vm_file != file) {
>> -				ret = -EINVAL;
>> -				break;
>> -			}
>> -			if (!file)
>> -				continue;
>> -			if (!vma_is_shmem(vmas[i]) && !is_file_hugepages(file)) {
>> -				ret = -EOPNOTSUPP;
>> -				break;
>> -			}
>> -		}
>> +		file = vma->vm_file;
>> +		if (file && !vma_is_shmem(vma) && !is_file_hugepages(file))
>> +			ret = -EOPNOTSUPP;
>> +
> 
> Also, why is it doing this?

There were problems with filesystem mappings, I believe.
Jens may remember what it was.


> All GUP users don't work entirely right for any fops implementation
> that assumes write protect is unconditionally possible. eg most
> filesystems.
> 
> We've been ignoring blocking it because it is an ABI break and it does
> sort of work in some cases.
> 
> I'd rather see something like FOLL_ALLOW_BROKEN_FILE_MAPPINGS than
> io_uring open coding this kind of stuff.

-- 
Pavel Begunkov