Re: [PATCH] mm: remove WARN_ON_ONCE() in file_has_valid_mmap_hooks()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, May 14, 2025 at 11:10:15AM +0200, David Hildenbrand wrote:
> 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@intel.com>
> > > > Closes: https://lore.kernel.org/oe-lkp/202505141434.96ce5e5d-lkp@intel.com
> > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.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.

Right, but nobody expected such a trivial change to be a problem.

However, having spoken to Pedro off-list, it's really obvious this could
happen, by trying to mmap() literally any file that's not un-mmap()-able, I
guess we all of us brain farted on this... :)

I'm keen for this to land for the next cycle, as I have a ton of follow up
work to do, and delaying that by a couple months would be deeply painful.

But sure a couple days would have been fine... :)

As hinted at at LSF, I'm in favour of a highly formulaic approach to all
this 'do X, get Y', so an amount of time in mm-unstable etc. could be part
of that.

Not that I'm saying we should replace Andrew with a script :P (sorry
Andrew!!) but that you know if he were script-like, then everything would
be super clear.

Of course you get endless edge cases that require a non-script entity to be
involved but in any case... :)

>
> >
> > 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.

OK wasn't aware of this, this is the information I was missing here thanks!

>
> --
> Cheers,
>
> David / dhildenb
>