Message-ID: <f3d52fe7-991f-4fd1-a326-6e8bfe54ddec@redhat.com>
Date: Wed, 14 May 2025 11:10:15 +0200
From: David Hildenbrand <david@...hat.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>,
 Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
 Jann Horn <jannh@...gle.com>, Pedro Falcato <pfalcato@...e.de>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH] mm: remove WARN_ON_ONCE() in file_has_valid_mmap_hooks()

On 14.05.25 10:56, Lorenzo Stoakes wrote:
> On Wed, May 14, 2025 at 10:49:57AM +0200, David Hildenbrand wrote:
>> On 14.05.25 10:40, Lorenzo Stoakes wrote:
>>> Having encountered a trinity report in linux-next (Linked in the 'Closes'
>>> tag) it appears that there are legitimate situations where a file-backed
>>> mapping can be acquired but no file->f_op->mmap or file->f_op->mmap_prepare
>>> is set, at which point do_mmap() should simply error out with -ENODEV.
>>>
>>> Since previously we did not warn in this scenario and it appears we rely
>>> upon this, restore this situation, while retaining a WARN_ON_ONCE() for the
>>> case where both are set, which is absolutely incorrect and must be
>>> addressed and thus always requires a warning.
>>>
>>> If further work is required to chase down precisely what is causing this,
>>> then we can later restore this, but it makes no sense to hold up this
>>> series to do so, as this is existing and apparently expected behaviour.
>>>
>>> Reported-by: kernel test robot <oliver.sang@...el.com>
>>> Closes: https://lore.kernel.org/oe-lkp/202505141434.96ce5e5d-lkp@intel.com
>>> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>>> ---
>>>
>>> Andrew -
>>>
>>> Since this series is in mm-stable we should take this fix there asap (and
>>> certainly get it to -next to fix any further error reports). I didn't know
>>> whether it was best for it to be a fix-patch or not, so have sent
>>> separately so you can best determine what to do with it :)
>>
>> A couple more days in mm-unstable probably wouldn't have hurt here,
>> especially given that I recall reviewing + seeing review yesterday?
>>
> 
> We're coming close to end of cycle, and the review commentary is essentially
> style stuff or follow up stuff, and also the series has a ton of tags now, so I
> - respectfully (you know I love you man :>) - disagree with this assessment :)
> 
> This situation that arose here is just extremely weird, there's really no reason
> anybody should rely on this scenario (yes we should probably try and chase this
> down actually, perhaps though a driver somehow sets f_op->mmap to NULL somewhere
> in some situation?)
> 
> So I think this (easily fixed) situation doesn't argue _too_ much against that
> :)

Again, I am talking about a couple more days, not weeks or months ;)

At least looking at the report it sounds like something the test bots 
would usually find given a bit more time on -next. I might be wrong.

next-20250500 had the old version without WARN

next-20250512 had the new version  with WARN

So the new version has been in -next (looks at calendar) .... for a 
short time.

> 
> But I take your point obviously!
> 
>> Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
> 
> Is it worth having a fixes tag for something not upstream? This is why I
> excluded that. I feel like it's maybe more misleading when the commit hashes are
> ephemeral in a certain branch?

mm-stable is supposed to have stable commit ids (unless Andrew rebases), 
so we usually use Fixes tags.

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists