If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
address (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.
Cc: Ira Weiny <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Suggested-by: Al Viro <[email protected]>
Confirmed-by: Helge Deller <[email protected]>
Confirmed-by: Matthew Wilcox <[email protected]>
Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*")
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
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?
2) I'm not sure whether or not the "Fixes" tag is appropriate in this
patch. Can someone either confirm or deny it?
include/linux/highmem-internal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index 034b1106d022..e247c9ac4583 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -200,7 +200,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
static inline void __kunmap_local(const void *addr)
{
#ifdef ARCH_HAS_FLUSH_ON_KUNMAP
- kunmap_flush_on_unmap(addr);
+ kunmap_flush_on_unmap(PTR_ALIGN_DOWN(addr, PAGE_SIZE));
#endif
}
--
2.39.0
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.
> If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> address (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.
>
> Cc: Ira Weiny <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Suggested-by: Al Viro <[email protected]>
> Confirmed-by: Helge Deller <[email protected]>
> Confirmed-by: Matthew Wilcox <[email protected]>
> Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*")
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> 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.
>
> 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.
I do wonder if this should be cc'ed to stable to ensure it gets
backported? Helge do you think there is a need for that?
Ira
>
> include/linux/highmem-internal.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
> index 034b1106d022..e247c9ac4583 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -200,7 +200,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
> static inline void __kunmap_local(const void *addr)
> {
> #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> - kunmap_flush_on_unmap(addr);
> + kunmap_flush_on_unmap(PTR_ALIGN_DOWN(addr, PAGE_SIZE));
> #endif
> }
>
> --
> 2.39.0
>
>
On Thu, Jan 26, 2023 at 03:33:46PM +0100, Fabio M. De Francesco wrote:
> If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> address (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.
You missed a spot. Sent the version I've had in my tree for a few days.
On 1/26/23 20:50, 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.
>
>> If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
>> calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
>> address (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.
>>
>> Cc: Ira Weiny <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Suggested-by: Al Viro <[email protected]>
>> Confirmed-by: Helge Deller <[email protected]>
>> Confirmed-by: Matthew Wilcox <[email protected]>
>> Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*")
>> Signed-off-by: Fabio M. De Francesco <[email protected]>
>> ---
>>
>> 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.
>
>>
>> 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.
It seems nobody is running highmem on parisc, because it can't be enabled.
AFAICS, it's not in any parisc related Kconfig file.
> I do wonder if this should be cc'ed to stable to ensure it gets
> backported? Helge do you think there is a need for that?
For correctness I think it's nevertheless good to backport it.
>> include/linux/highmem-internal.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
>> index 034b1106d022..e247c9ac4583 100644
>> --- a/include/linux/highmem-internal.h
>> +++ b/include/linux/highmem-internal.h
>> @@ -200,7 +200,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
>> static inline void __kunmap_local(const void *addr)
>> {
>> #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
>> - kunmap_flush_on_unmap(addr);
>> + kunmap_flush_on_unmap(PTR_ALIGN_DOWN(addr, PAGE_SIZE));
>> #endif
>> }
That would be another possibility:
diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
index 0bdee6724132..ce5d1f8a23bd 100644
--- a/arch/parisc/include/asm/cacheflush.h
+++ b/arch/parisc/include/asm/cacheflush.h
@@ -77,6 +77,7 @@ void flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned lon
#define ARCH_HAS_FLUSH_ON_KUNMAP
static inline void kunmap_flush_on_unmap(const void *addr)
{
+ addr = PTR_ALIGN_DOWN(addr, PAGE_SIZE);
flush_kernel_dcache_page_addr(addr);
}
Helge
On Thu, 26 Jan 2023 15:33:46 +0100 "Fabio M. De Francesco" <[email protected]> wrote:
> If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> address (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.
What are the user-visible runtime effects of this flaw?
> 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.
>
> Cc: Ira Weiny <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Suggested-by: Al Viro <[email protected]>
> Confirmed-by: Helge Deller <[email protected]>
> Confirmed-by: Matthew Wilcox <[email protected]>
> Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*")
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
>
> 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?
Add a paragraph "this was confirmed by X and Y", then add Cc:X, Cc:y?
This gives you an opportunity to tell us what "confirmed" actually
means! Did they confirm that it's a bug? Or that the fix is correct?
I dunno.
> 2) I'm not sure whether or not the "Fixes" tag is appropriate in this
> patch. Can someone either confirm or deny it?
>
> include/linux/highmem-internal.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
> index 034b1106d022..e247c9ac4583 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -200,7 +200,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
> static inline void __kunmap_local(const void *addr)
> {
> #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> - kunmap_flush_on_unmap(addr);
> + kunmap_flush_on_unmap(PTR_ALIGN_DOWN(addr, PAGE_SIZE));
> #endif
> }
>
> --
> 2.39.0
On Thu, Jan 26, 2023 at 12:38:58PM -0800, Andrew Morton wrote:
> On Thu, 26 Jan 2023 15:33:46 +0100 "Fabio M. De Francesco" <[email protected]> wrote:
>
> > If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > address (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.
>
> What are the user-visible runtime effects of this flaw?
The version of this patch I sent out includes this information,
as well as the missed alignment for kunmap_atomic().
On Thu, Jan 26, 2023 at 09:37:08PM +0100, Helge Deller wrote:
> > 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.
>
> It seems nobody is running highmem on parisc, because it can't be enabled.
> AFAICS, it's not in any parisc related Kconfig file.
But this isn't being used for highmem on parisc; it's being used for
cache coherency.
> > I do wonder if this should be cc'ed to stable to ensure it gets
> > backported? Helge do you think there is a need for that?
>
> For correctness I think it's nevertheless good to backport it.
I cc'd stable on my version of this patch, and included a Fixes tag
to indicate how far back to backport it.
> That would be another possibility:
>
> diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
> index 0bdee6724132..ce5d1f8a23bd 100644
> --- a/arch/parisc/include/asm/cacheflush.h
> +++ b/arch/parisc/include/asm/cacheflush.h
> @@ -77,6 +77,7 @@ void flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned lon
> #define ARCH_HAS_FLUSH_ON_KUNMAP
> static inline void kunmap_flush_on_unmap(const void *addr)
> {
> + addr = PTR_ALIGN_DOWN(addr, PAGE_SIZE);
I considered that, but thought it a shame to do it here when all the
other users are passing in a page-aligned address.
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 <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Suggested-by: Al Viro <[email protected]>
> > Confirmed-by: Helge Deller <[email protected]>
> > Confirmed-by: Matthew Wilcox <[email protected]>
> > Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*")
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
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.
On Thu, Jan 26, 2023 at 08:48:03PM +0000, Matthew Wilcox wrote:
> On Thu, Jan 26, 2023 at 12:38:58PM -0800, Andrew Morton wrote:
> > On Thu, 26 Jan 2023 15:33:46 +0100 "Fabio M. De Francesco" <[email protected]> wrote:
> >
> > > If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> > > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > > address (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.
> >
> > What are the user-visible runtime effects of this flaw?
>
> The version of this patch I sent out includes this information,
> as well as the missed alignment for kunmap_atomic().
One point: AFAICS, the situation right now is
* all callers of kunmap_local() pass page-aligned pointers
* all callers of kunmap_atomic() seem to do the same
* there's nothing in documentation that would say one can
pass anything other than the return value of original kmap_local_page()
or kmap_atomic() call to those.
* there's nothing that would outright ban doing that.
So these patches really ought to touch Documentation/mm/highmem.rst
saying that from now on kunmap_local() and kunmap_atomic() callers
are allowed to pass any pointer within the mapped area. And yes,
we want it in -stable before anything that relies upon that sucker
gets backported.
On Thu, Jan 26, 2023 at 09:04:02PM +0000, Al Viro wrote:
> On Thu, Jan 26, 2023 at 08:48:03PM +0000, Matthew Wilcox wrote:
> > On Thu, Jan 26, 2023 at 12:38:58PM -0800, Andrew Morton wrote:
> > > On Thu, 26 Jan 2023 15:33:46 +0100 "Fabio M. De Francesco" <[email protected]> wrote:
> > >
> > > > If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> > > > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > > > address (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.
> > >
> > > What are the user-visible runtime effects of this flaw?
> >
> > The version of this patch I sent out includes this information,
> > as well as the missed alignment for kunmap_atomic().
>
> One point: AFAICS, the situation right now is
> * all callers of kunmap_local() pass page-aligned pointers
Ah, no. kmap_local_folio() accepts a byte offset within the folio
and returns a pointer to that byte. I hadn't noticed the parisc
case and thought it was already allowed to pass a misaligned pointer
to kunmap_local() since it is allowed for the highmem case. It
simplified the callers, so it looked like a good tradeoff.
See, eg 338f379cf7c2:
- src_addr = kmap_atomic(src_page);
- dest_addr = kmap_atomic(dest_page);
+ src_addr = kmap_local_folio(src_folio,
+ offset_in_folio(src_folio, srcoff));
+ dst_addr = kmap_local_folio(dst_folio,
+ offset_in_folio(dst_folio, dstoff));
- if (memcmp(src_addr + src_poff, dest_addr + dest_poff, cmp_len))
+ if (memcmp(src_addr, dst_addr, cmp_len))
- kunmap_atomic(dest_addr);
- kunmap_atomic(src_addr);
+ kunmap_local(dst_addr);
+ kunmap_local(src_addr);
On Thu, Jan 26, 2023 at 09:56:07PM +0000, Matthew Wilcox wrote:
> On Thu, Jan 26, 2023 at 09:04:02PM +0000, Al Viro wrote:
> > On Thu, Jan 26, 2023 at 08:48:03PM +0000, Matthew Wilcox wrote:
> > > On Thu, Jan 26, 2023 at 12:38:58PM -0800, Andrew Morton wrote:
> > > > On Thu, 26 Jan 2023 15:33:46 +0100 "Fabio M. De Francesco" <[email protected]> wrote:
> > > >
> > > > > If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> > > > > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > > > > address (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.
> > > >
> > > > What are the user-visible runtime effects of this flaw?
> > >
> > > The version of this patch I sent out includes this information,
> > > as well as the missed alignment for kunmap_atomic().
> >
> > One point: AFAICS, the situation right now is
> > * all callers of kunmap_local() pass page-aligned pointers
>
> Ah, no. kmap_local_folio() accepts a byte offset within the folio
> and returns a pointer to that byte. I hadn't noticed the parisc
> case and thought it was already allowed to pass a misaligned pointer
> to kunmap_local() since it is allowed for the highmem case. It
> simplified the callers, so it looked like a good tradeoff.
>
> See, eg 338f379cf7c2:
>
> - src_addr = kmap_atomic(src_page);
> - dest_addr = kmap_atomic(dest_page);
> + src_addr = kmap_local_folio(src_folio,
> + offset_in_folio(src_folio, srcoff));
> + dst_addr = kmap_local_folio(dst_folio,
> + offset_in_folio(dst_folio, dstoff));
>
> - if (memcmp(src_addr + src_poff, dest_addr + dest_poff, cmp_len))
> + if (memcmp(src_addr, dst_addr, cmp_len))
>
> - kunmap_atomic(dest_addr);
> - kunmap_atomic(src_addr);
> + kunmap_local(dst_addr);
> + kunmap_local(src_addr);
Argh... Missed that.
OK, then Fixes: on the patch makes a lot of sense. And it still
needs a chunk in D/m/highmem.rst...
On gioved? 26 gennaio 2023 21:07:53 CET Matthew Wilcox wrote:
> On Thu, Jan 26, 2023 at 03:33:46PM +0100, Fabio M. De Francesco wrote:
> > If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > address (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.
>
> You missed a spot. Sent the version I've had in my tree for a few days.
I'm sorry for missing the other call site.
Furthermore, your patch has a better commit message.
However, I'm also sorry because I would have expected a different kind of
support from you. My patch could have been improved with v2.
Anyway, I assume that you missed that I had asked you and Al if anybody were
interested in doing these changes and probably you missed also that Ira
encouraged me to send a patch, whereas you did not show any interest...
https://lore.kernel.org/lkml/3146373.5fSG56mABF@suse/
Fabio
P.S.: Helge wrote to me privately but you cannot know it.
On Fri, Jan 27, 2023 at 06:58:55PM +0100, Fabio M. De Francesco wrote:
> On gioved? 26 gennaio 2023 21:07:53 CET Matthew Wilcox wrote:
> > On Thu, Jan 26, 2023 at 03:33:46PM +0100, Fabio M. De Francesco wrote:
> > > If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> > > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > > address (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.
> >
> > You missed a spot. Sent the version I've had in my tree for a few days.
>
> I'm sorry for missing the other call site.
> Furthermore, your patch has a better commit message.
>
> However, I'm also sorry because I would have expected a different kind of
> support from you. My patch could have been improved with v2.
>
> Anyway, I assume that you missed that I had asked you and Al if anybody were
> interested in doing these changes and probably you missed also that Ira
> encouraged me to send a patch, whereas you did not show any interest...
> https://lore.kernel.org/lkml/3146373.5fSG56mABF@suse/
Sorry; I get too much email and I missed the last six emails in that
thread (just went back and read them). According to git, I committed
this patch to my tree on Jan 20th, so if I'd read your email, I would
have told you to not bother.
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 <[email protected]>
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 <[email protected]>
Reviewed-by: Martin K. Petersen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jens Axboe <[email protected]>
> 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
Matthew Wilcox wrote:
> On Thu, Jan 26, 2023 at 09:37:08PM +0100, Helge Deller wrote:
> > > 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.
> >
> > It seems nobody is running highmem on parisc, because it can't be enabled.
> > AFAICS, it's not in any parisc related Kconfig file.
>
> But this isn't being used for highmem on parisc; it's being used for
> cache coherency.
<sigh> right! I missed that. Thanks to you and Al for setting me
straight.
>
> > > I do wonder if this should be cc'ed to stable to ensure it gets
> > > backported? Helge do you think there is a need for that?
> >
> > For correctness I think it's nevertheless good to backport it.
>
> I cc'd stable on my version of this patch, and included a Fixes tag
> to indicate how far back to backport it.
Thanks!
>
> > That would be another possibility:
> >
> > diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
> > index 0bdee6724132..ce5d1f8a23bd 100644
> > --- a/arch/parisc/include/asm/cacheflush.h
> > +++ b/arch/parisc/include/asm/cacheflush.h
> > @@ -77,6 +77,7 @@ void flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned lon
> > #define ARCH_HAS_FLUSH_ON_KUNMAP
> > static inline void kunmap_flush_on_unmap(const void *addr)
> > {
> > + addr = PTR_ALIGN_DOWN(addr, PAGE_SIZE);
>
> I considered that, but thought it a shame to do it here when all the
> other users are passing in a page-aligned address.
>
I think this also keeps the kunmap_local() API intact better.
Patch reviewed.
Thanks!
Ira
Al Viro wrote:
> On Thu, Jan 26, 2023 at 09:56:07PM +0000, Matthew Wilcox wrote:
> > On Thu, Jan 26, 2023 at 09:04:02PM +0000, Al Viro wrote:
> > > On Thu, Jan 26, 2023 at 08:48:03PM +0000, Matthew Wilcox wrote:
> > > > On Thu, Jan 26, 2023 at 12:38:58PM -0800, Andrew Morton wrote:
> > > > > On Thu, 26 Jan 2023 15:33:46 +0100 "Fabio M. De Francesco" <[email protected]> wrote:
> > > > >
> > > > > > If ARCH_HAS_FLUSH_ON_KUNMAP is defined (PA-RISC case), __kunmap_local()
> > > > > > calls kunmap_flush_on_unmap(). The latter currently flushes the wrong
> > > > > > address (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.
> > > > >
> > > > > What are the user-visible runtime effects of this flaw?
> > > >
> > > > The version of this patch I sent out includes this information,
> > > > as well as the missed alignment for kunmap_atomic().
> > >
> > > One point: AFAICS, the situation right now is
> > > * all callers of kunmap_local() pass page-aligned pointers
> >
> > Ah, no. kmap_local_folio() accepts a byte offset within the folio
> > and returns a pointer to that byte. I hadn't noticed the parisc
> > case and thought it was already allowed to pass a misaligned pointer
> > to kunmap_local() since it is allowed for the highmem case. It
> > simplified the callers, so it looked like a good tradeoff.
> >
> > See, eg 338f379cf7c2:
> >
> > - src_addr = kmap_atomic(src_page);
> > - dest_addr = kmap_atomic(dest_page);
> > + src_addr = kmap_local_folio(src_folio,
> > + offset_in_folio(src_folio, srcoff));
> > + dst_addr = kmap_local_folio(dst_folio,
> > + offset_in_folio(dst_folio, dstoff));
> >
> > - if (memcmp(src_addr + src_poff, dest_addr + dest_poff, cmp_len))
> > + if (memcmp(src_addr, dst_addr, cmp_len))
> >
> > - kunmap_atomic(dest_addr);
> > - kunmap_atomic(src_addr);
> > + kunmap_local(dst_addr);
> > + kunmap_local(src_addr);
>
> Argh... Missed that.
>
> OK, then Fixes: on the patch makes a lot of sense. And it still
> needs a chunk in D/m/highmem.rst...
>
I believe this is taken care of because the kdocs for
kunmap_{local,atomic}() are included there. And those kdocs indicate the
ability to use any address in the page.
Thanks for setting me straight on the parisc/highmem thing I missed that.
Ira