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@xxxxxxxxx> > > > > Closes: https://lore.kernel.org/oe-lkp/202505141434.96ce5e5d-lkp@xxxxxxxxx > > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> > > > > --- > > > > > > > > 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 >
