Re: [PATCH] mm/highmem: Align-down to page the address for kunmap_flush_on_unmap()

[Al Viro]

On Thu, Jan 26, 2023 at 11:50:19AM -0800, Ira Weiny wrote:
> Fabio M. De Francesco wrote:
> 
> FWIW I think I would simplify the subject
> [PATCH] mm/highmem: Fix kunmap_local() on flush on unmap architectures
> 
> Or something like that.

Make kunmap_local() handle addresses that are not page-aligned

kunmap_local() removes the temporary CPU-local mapping of a page that
had been created by earlier call of kmap_local_page().  The mapping to
be removed is identified by the pointer returned by kmap_local_page(),
i.e. the virtual address assigned to the first byte within the page
in question.  Often enough the callers had been working with an object
somewhere in the middle of the page; they have to either keep the
pointer to the beginning or to round the pointers they are really
working with down to the beginning of page.

As it is, for the majority of architectures kunmap_local() does such
rounding-down anyway (see kunmap_local_indexed()); the only exception
is if CONFIG_HIGHMEM is *not* defined, but ARCH_HAS_FLUSH_ON_KUNMAP is
(PA-RISC case).  In that case __kumap_local()

> > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > address

if given a pointer that is not page-aligned

> > (as confirmed by Matthew Wilcox and Helge Deller). Al Viro
> > proposed to call kunmap_flush_on_unmap() on an aligned-down to page
> > address in order to fix this issue. Consensus has been reached on this
> > solution.
> > 
> > Therefore, if ARCH_HAS_FLUSH_ON_KUNMAP is defined, call
> > kunmap_flush_on_unmap() on an aligned-down to page address computed with
> > the PTR_ALIGN_DOWN() macro.

That simplifies life for callers (e.g. filesystems that use kmap_local_page()
for directory page cache).

> > 
> > Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Confirmed-by: Helge Deller <deller@xxxxxx>
> > Confirmed-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*")
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>

Objections?

> > 
> > I have (at least) two problems with this patch...
> > 
> > 1) checkpatch.pl complains about the use of the non-standard
> > "Confirmed-by" tags. I don't know how else I can give credit to Helge
> > and Matthew. However, this is not the first time that I see non-standard
> > tags in patches applied upstream (I too had a non-standard
> > "Analysed-by" tag in patch which fixes a SAC bug). Any objections?
> 
> I think you can add Matthew and Helge as Suggested-by:  All 3 had input on
> the solution.

Or simply treat these complaints as per https://dilbert.com/strip/1996-10-04...

> > 2) I'm not sure whether or not the "Fixes" tag is appropriate in this
> > patch. Can someone either confirm or deny it?
> 
> This 'fixes' looks correct to me.  I don't know how many folks are running
> highmem with parisc but if they are I am sure they would appreciate the
> extra knowledge.

parisc doesn't have highmem.  That's not what flush_kernel_dcache_page_addr()
call is about; if you look at the kmap() there you'll see that it does
*not* create any new mapping - it simply returns page_address().  It's
really about D-cache flushing there; kunmap() and its ilk serve as
convenient points for doing the flush.

Not sure about the "fixes" tag, TBH - AFAICS, currently all users supply
page-aligned addresses anyway.  Their life would be easier if they could
just pass any pointer within the page in questions and it's easy to overlook
this corner case and assume that kunmap_local() would handle that as it
is, but it's more of a "bug waiting to happen, better get rid of the
corner case and explicitly document that property of kunmap_local()" than
"there's a broken caller in the current mainline kernel, need to fix
that".

Anyway, I would rather keep the sysv etc. series independent from that;
for now dir_put_page() explicitly rounds down there.  Once those series
*and* your patch are both merged we can do a quick followup removing the
explicit round-downs, marked as dependent upon your patch.