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 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
:)

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?

>
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks!

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