| Message ID | 20230126143346.12086-1-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/highmem: Align-down to page the address for kunmap_flush_on_unmap() | expand |
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 <ira.weiny@intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk> > Confirmed-by: Helge Deller <deller@gmx.de> > Confirmed-by: Matthew Wilcox <willy@infradead.org> > Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*") > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > > 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 <ira.weiny@intel.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Suggested-by: Al Viro <viro@zeniv.linux.org.uk> >> Confirmed-by: Helge Deller <deller@gmx.de> >> Confirmed-by: Matthew Wilcox <willy@infradead.org> >> Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*") >> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> >> --- >> >> 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" <fmdefrancesco@gmail.com> 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 <ira.weiny@intel.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk> > Confirmed-by: Helge Deller <deller@gmx.de> > Confirmed-by: Matthew Wilcox <willy@infradead.org> > Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*") > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > > 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" <fmdefrancesco@gmail.com> 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 <ira.weiny@intel.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk> > > Confirmed-by: Helge Deller <deller@gmx.de> > > Confirmed-by: Matthew Wilcox <willy@infradead.org> > > Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*") > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> 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" <fmdefrancesco@gmail.com> 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" <fmdefrancesco@gmail.com> 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" <fmdefrancesco@gmail.com> 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 <hch@lst.de> 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@lst.de> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com> Link: https://lore.kernel.org/r/20210727055646.118787-15-hch@lst.de Signed-off-by: Jens Axboe <axboe@kernel.dk> > 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" <fmdefrancesco@gmail.com> 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
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 }
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 <ira.weiny@intel.com> Cc: Thomas Gleixner <tglx@linutronix.de> Suggested-by: Al Viro <viro@zeniv.linux.org.uk> Confirmed-by: Helge Deller <deller@gmx.de> Confirmed-by: Matthew Wilcox <willy@infradead.org> Fixes: f3ba3c710ac5 ("mm/highmem: Provide kmap_local*") Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- 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(-)