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

[Ira Weiny]

Al Viro wrote:
> On Thu, Jan 26, 2023 at 11:50:19AM -0800, Ira Weiny wrote:

[snip]

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

Not all users supply page-aligned addresses.  Partly because
kunmap_local() was documented to not require it.  But more importantly
because those of us doing the conversions were focused on what would
happen in the highmem case.  So we did not catch this side
effect/requirement in flush_kernel_dcache_page_addr().  :-/

kunmap_atomic:

 * @__addr can be any address within the mapped page, so there is no need
 * to subtract any offset that has been added. In contrast to kunmap(),
 * this function takes the address returned from kmap_atomic(), not the
 * page passed to it. The compiler will warn you if you pass the page.

kunmap_local:

 * @__addr can be any address within the mapped page.  Commonly it is the
 * address return from kmap_local_page(), but it can also include offsets.

> 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".
 
FWIW there are callers like this in mainline.  Here is an example.

commit 8aec120a9ca80c14ce002505cea1e1639f8e9ea5                                           
Author: Christoph Hellwig <hch@xxxxxx>                                                    
Date:   Tue Jul 27 07:56:45 2021 +0200                                                    
                                                                             
    block: use bvec_kmap_local in t10_pi_type1_{prepare,complete}                         
                                                                     
    Using local kmaps slightly reduces the chances to stray writes, and                   
    the bvec interface cleans up the code a little bit.                      
                                                              
    Signed-off-by: Christoph Hellwig <hch@xxxxxx>                                         
    Reviewed-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>                          
    Link: https://lore.kernel.org/r/20210727055646.118787-15-hch@xxxxxx 
    Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>                                           


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

This seems reasonable.

Again, thanks for all your help Al!
Ira