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

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

[Lorenzo Stoakes]

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

Thanks, Lorenzo

 include/linux/fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index e2721a1ff13d..09c8495dacdb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2248,7 +2248,7 @@ static inline bool file_has_valid_mmap_hooks(struct file *file)
 	/* Hooks are mutually exclusive. */
 	if (WARN_ON_ONCE(has_mmap && has_mmap_prepare))
 		return false;
-	if (WARN_ON_ONCE(!has_mmap && !has_mmap_prepare))
+	if (!has_mmap && !has_mmap_prepare)
 		return false;

 	return true;
--
2.49.0

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

[Vlastimil Babka]

On 5/14/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>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> ---
> 
> 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 :)
> 
> Thanks, Lorenzo
> 
>  include/linux/fs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e2721a1ff13d..09c8495dacdb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2248,7 +2248,7 @@ static inline bool file_has_valid_mmap_hooks(struct file *file)
>  	/* Hooks are mutually exclusive. */
>  	if (WARN_ON_ONCE(has_mmap && has_mmap_prepare))
>  		return false;
> -	if (WARN_ON_ONCE(!has_mmap && !has_mmap_prepare))
> +	if (!has_mmap && !has_mmap_prepare)
>  		return false;
> 
>  	return true;
> --
> 2.49.0

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

[Pedro Falcato]

On Wed, May 14, 2025 at 10:42:53AM +0200, Vlastimil Babka wrote:
> On 5/14/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>
> 
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

Reviewed-by: Pedro Falcato <pfalcato@suse.de>

-- 
Pedro

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

[David Hildenbrand]

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?

Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")

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

-- 
Cheers,

David / dhildenb

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

[Lorenzo Stoakes]

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
>

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

[David Hildenbrand]

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.

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

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

[David Hildenbrand]

On 14.05.25 11:10, 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

^ for completeness, next-20250509

-- 
Cheers,

David / dhildenb

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

[Lorenzo Stoakes]

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
>

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

[Vlastimil Babka]

On 5/14/25 10:56, Lorenzo Stoakes wrote:
>> Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")

Ah yeah I missed there wasn't one.

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

Yeah it can be useful, in case the fixed commit gets backported somewhere,
tools can warn that there's a follow up fix. As mm-stable hashes should not
be ephemeral, then this should remain valid (and if there's a rebase for
some reason then the fix could be squashed).

>>
>> Acked-by: David Hildenbrand <david@redhat.com>
> 
> Thanks!
> 
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>

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

[Lorenzo Stoakes]

On Wed, May 14, 2025 at 11:13:27AM +0200, Vlastimil Babka wrote:
> On 5/14/25 10:56, Lorenzo Stoakes wrote:
> >> Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
>
> Ah yeah I missed there wasn't one.
>
> > 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?
>
> Yeah it can be useful, in case the fixed commit gets backported somewhere,
> tools can warn that there's a follow up fix. As mm-stable hashes should not
> be ephemeral, then this should remain valid (and if there's a rebase for
> some reason then the fix could be squashed).

Yeah the mm-stable hashes not being ephemeral was the bit of information I
was missing, I would have added this tag otherwise!

One day I'll get the hang of this mm stuff... :P

>
> >>
> >> Acked-by: David Hildenbrand <david@redhat.com>
> >
> > Thanks!
> >
> >>
> >> --
> >> Cheers,
> >>
> >> David / dhildenb
> >>
>
>

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

[Lorenzo Stoakes]

Andrew:

(to save you having to dive into rest of thread)

Could you add the Fixes line suggested by David below?

Thanks! :)

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?
>
> Fixes: c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file callback")
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
> --
> Cheers,
>
> David / dhildenb
>