linux-kernel - Re: [PATCH] mm: remove WARN_ON_ONCE() in file_has_valid_mmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <729bc3ef-149b-45d0-aee0-d199050f0122@lucifer.local>
Date: Wed, 14 May 2025 10:18:55 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.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 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@...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.

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
>