[V2,4/8] mm/highmem: Add VM_BUG_ON() to mem*_page() calls
diff mbox series

Message ID 20210210062221.3023586-5-ira.weiny@intel.com
State New, archived
Headers show
Series
  • btrfs: convert kmaps to core page calls
Related show

Commit Message

Ira Weiny Feb. 10, 2021, 6:22 a.m. UTC
From: Ira Weiny <ira.weiny@intel.com>

Add VM_BUG_ON bounds checks to ensure the newly lifted and created page
memory operations do not result in corrupted data in neighbor pages and
to make them consistent with zero_user().[1][2]

[1] https://lore.kernel.org/lkml/20201210053502.GS1563847@iweiny-DESK2.sc.intel.com/
[2] https://lore.kernel.org/lkml/20210209110931.00f00e47d9a0529fcee2ff01@linux-foundation.org/

Cc: Christoph Hellwig <hch@infradead.org>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
New for V2
---
 include/linux/highmem.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Chaitanya Kulkarni Feb. 10, 2021, 6:57 a.m. UTC | #1
On 2/9/21 22:25, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
>
> Add VM_BUG_ON bounds checks to ensure the newly lifted and created page
> memory operations do not result in corrupted data in neighbor pages and
> to make them consistent with zero_user().[1][2]
>
I did not understand this, in my tree :-

zero_user()
 -> zero_user_segments()

which uses BUG_ON(), the commit log says add VM_BUG_ON(), isn't that
inconsistent withwhat is there in zero_user_segments() which uses BUG_ON() ?

Also, this patch uses BUG_ON() which doesn't match the commit log that says
ADD VM_BUG_ON(),

Did I interpret the commit log wrong ?

[1]
 void zero_user_segments(struct page *page, unsigned start1, unsigned end1,
365                 unsigned start2, unsigned end2)
366 {
367         unsigned int
i;                                                                           

368
369         BUG_ON(end1 > page_size(page) || end2 > page_size(page));
370
371         for (i = 0; i < compound_nr(page); i++) {
372                 void *kaddr = NULL;
373 
374                 if (start1 < PAGE_SIZE || start2 < PAGE_SIZE)
375                         kaddr = kmap_atomic(page + i);
376
377                 if (start1 >= PAGE_SIZE) {
378                         start1 -= PAGE_SIZE;
379                         end1 -= PAGE_SIZE;
380                 } else {
381                         unsigned this_end = min_t(unsigned, end1,
PAGE_SIZE);
382        
383                         if (end1 > start1)
384                                 memset(kaddr + start1, 0, this_end -
start1);
385                         end1 -= this_end;
386                         start1 = 0;
387                 }
388
389                 if (start2 >= PAGE_SIZE) {
390                         start2 -= PAGE_SIZE;
391                         end2 -= PAGE_SIZE;
392                 } else {
393                         unsigned this_end = min_t(unsigned, end2,
PAGE_SIZE);
394 
395                         if (end2 > start2)
396                                 memset(kaddr + start2, 0, this_end -
start2);
397                         end2 -= this_end;
398                         start2 = 0;
399                 }
400        
401                 if (kaddr) {
402                         kunmap_atomic(kaddr);
403                         flush_dcache_page(page + i);
404                 }
405        
406                 if (!end1 && !end2)
407                         break;
408         }
409        
410         BUG_ON((start1 | start2 | end1 | end2) != 0);
411 }
412 EXPORT_SYMBOL(zero_user_segments);
Christoph Hellwig Feb. 10, 2021, 12:55 p.m. UTC | #2
On Tue, Feb 09, 2021 at 10:22:17PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Add VM_BUG_ON bounds checks to ensure the newly lifted and created page
> memory operations do not result in corrupted data in neighbor pages and
> to make them consistent with zero_user().[1][2]

s/VM_BUG_ON/BUG_ON/g ?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Ira Weiny Feb. 10, 2021, 4:29 p.m. UTC | #3
On Wed, Feb 10, 2021 at 12:55:02PM +0000, Christoph Hellwig wrote:
> On Tue, Feb 09, 2021 at 10:22:17PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Add VM_BUG_ON bounds checks to ensure the newly lifted and created page
> > memory operations do not result in corrupted data in neighbor pages and
> > to make them consistent with zero_user().[1][2]
> 
> s/VM_BUG_ON/BUG_ON/g ?

Andrew wanted VM_BUG_ON.[1]

And I thought it was a good idea.  Any file system development should have
tests with DEBUG_VM which should cover Matthew's concern while not having the
overhead in production.  Seemed like a decent compromise?

Ira

[1] https://lore.kernel.org/lkml/20210209131103.b46e80db675fec8bec8d2ad1@linux-foundation.org/

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Ira Weiny Feb. 10, 2021, 4:31 p.m. UTC | #4
On Wed, Feb 10, 2021 at 06:57:30AM +0000, Chaitanya Kulkarni wrote:
> On 2/9/21 22:25, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > Add VM_BUG_ON bounds checks to ensure the newly lifted and created page
> > memory operations do not result in corrupted data in neighbor pages and
> > to make them consistent with zero_user().[1][2]
> >
> I did not understand this, in my tree :-
> 
> zero_user()
>  -> zero_user_segments()
> 
> which uses BUG_ON(), the commit log says add VM_BUG_ON(), isn't that
> inconsistent withwhat is there in zero_user_segments() which uses BUG_ON() ?
> 
> Also, this patch uses BUG_ON() which doesn't match the commit log that says
> ADD VM_BUG_ON(),

Oops, yea that 'consistent with zero_user()' was carried over from the BUG_ON
commit comment in the original patch...

The comment should be deleted.

But I'm going to wait because Christoph prefers BUG_ON...

Ira

> 
> Did I interpret the commit log wrong ?
> 
> [1]
>  void zero_user_segments(struct page *page, unsigned start1, unsigned end1,
> 365                 unsigned start2, unsigned end2)
> 366 {
> 367         unsigned int
> i;                                                                           
> 
> 368
> 369         BUG_ON(end1 > page_size(page) || end2 > page_size(page));
> 370
> 371         for (i = 0; i < compound_nr(page); i++) {
> 372                 void *kaddr = NULL;
> 373 
> 374                 if (start1 < PAGE_SIZE || start2 < PAGE_SIZE)
> 375                         kaddr = kmap_atomic(page + i);
> 376
> 377                 if (start1 >= PAGE_SIZE) {
> 378                         start1 -= PAGE_SIZE;
> 379                         end1 -= PAGE_SIZE;
> 380                 } else {
> 381                         unsigned this_end = min_t(unsigned, end1,
> PAGE_SIZE);
> 382        
> 383                         if (end1 > start1)
> 384                                 memset(kaddr + start1, 0, this_end -
> start1);
> 385                         end1 -= this_end;
> 386                         start1 = 0;
> 387                 }
> 388
> 389                 if (start2 >= PAGE_SIZE) {
> 390                         start2 -= PAGE_SIZE;
> 391                         end2 -= PAGE_SIZE;
> 392                 } else {
> 393                         unsigned this_end = min_t(unsigned, end2,
> PAGE_SIZE);
> 394 
> 395                         if (end2 > start2)
> 396                                 memset(kaddr + start2, 0, this_end -
> start2);
> 397                         end2 -= this_end;
> 398                         start2 = 0;
> 399                 }
> 400        
> 401                 if (kaddr) {
> 402                         kunmap_atomic(kaddr);
> 403                         flush_dcache_page(page + i);
> 404                 }
> 405        
> 406                 if (!end1 && !end2)
> 407                         break;
> 408         }
> 409        
> 410         BUG_ON((start1 | start2 | end1 | end2) != 0);
> 411 }
> 412 EXPORT_SYMBOL(zero_user_segments);
> 
> 
>
Christoph Hellwig Feb. 10, 2021, 4:41 p.m. UTC | #5
On Wed, Feb 10, 2021 at 08:29:01AM -0800, Ira Weiny wrote:
> On Wed, Feb 10, 2021 at 12:55:02PM +0000, Christoph Hellwig wrote:
> > On Tue, Feb 09, 2021 at 10:22:17PM -0800, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > Add VM_BUG_ON bounds checks to ensure the newly lifted and created page
> > > memory operations do not result in corrupted data in neighbor pages and
> > > to make them consistent with zero_user().[1][2]
> > 
> > s/VM_BUG_ON/BUG_ON/g ?
> 
> Andrew wanted VM_BUG_ON.[1]

I don't care either way, but the patch uses BUG_ON, so the description
should match.
Ira Weiny Feb. 10, 2021, 5:04 p.m. UTC | #6
On Wed, Feb 10, 2021 at 04:41:34PM +0000, Christoph Hellwig wrote:
> On Wed, Feb 10, 2021 at 08:29:01AM -0800, Ira Weiny wrote:
> > On Wed, Feb 10, 2021 at 12:55:02PM +0000, Christoph Hellwig wrote:
> > > On Tue, Feb 09, 2021 at 10:22:17PM -0800, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > Add VM_BUG_ON bounds checks to ensure the newly lifted and created page
> > > > memory operations do not result in corrupted data in neighbor pages and
> > > > to make them consistent with zero_user().[1][2]
> > > 
> > > s/VM_BUG_ON/BUG_ON/g ?
> > 
> > Andrew wanted VM_BUG_ON.[1]
> 
> I don't care either way, but the patch uses BUG_ON, so the description
> should match.

Oh man...  I changed the commit message after spliting the patch and forgot to
change the code...  <doh>

Thanks,
Ira
Matthew Wilcox Feb. 10, 2021, 6:56 p.m. UTC | #7
On Wed, Feb 10, 2021 at 08:29:01AM -0800, Ira Weiny wrote:
> And I thought it was a good idea.  Any file system development should have
> tests with DEBUG_VM which should cover Matthew's concern while not having the
> overhead in production.  Seemed like a decent compromise?

Why do you think these paths are only used during file system development?
They're definitely used by networking, by device drivers of all kinds
and they're probably even used by the graphics system.

While developers *should* turn on DEBUG_VM during development, a
shockingly high percentage don't even turn on lockdep.
Ira Weiny Feb. 10, 2021, 9:22 p.m. UTC | #8
On Wed, Feb 10, 2021 at 06:56:06PM +0000, Matthew Wilcox wrote:
> On Wed, Feb 10, 2021 at 08:29:01AM -0800, Ira Weiny wrote:
> > And I thought it was a good idea.  Any file system development should have
> > tests with DEBUG_VM which should cover Matthew's concern while not having the
> > overhead in production.  Seemed like a decent compromise?
> 
> Why do you think these paths are only used during file system development?

I can't guarantee it but right now most of the conversions I have worked on are
in FS's.

> They're definitely used by networking, by device drivers of all kinds
> and they're probably even used by the graphics system.
> 
> While developers *should* turn on DEBUG_VM during development, a
> shockingly high percentage don't even turn on lockdep.

Honestly, I don't feel strongly enough to argue it.

Andrew?  David?  David this is going through your tree so would you feel more
comfortable with 1 or the other?

Ira
David Sterba Feb. 11, 2021, 6:52 p.m. UTC | #9
On Wed, Feb 10, 2021 at 01:22:28PM -0800, Ira Weiny wrote:
> On Wed, Feb 10, 2021 at 06:56:06PM +0000, Matthew Wilcox wrote:
> > On Wed, Feb 10, 2021 at 08:29:01AM -0800, Ira Weiny wrote:
> > > And I thought it was a good idea.  Any file system development should have
> > > tests with DEBUG_VM which should cover Matthew's concern while not having the
> > > overhead in production.  Seemed like a decent compromise?
> > 
> > Why do you think these paths are only used during file system development?
> 
> I can't guarantee it but right now most of the conversions I have worked on are
> in FS's.
> 
> > They're definitely used by networking, by device drivers of all kinds
> > and they're probably even used by the graphics system.
> > 
> > While developers *should* turn on DEBUG_VM during development, a
> > shockingly high percentage don't even turn on lockdep.
> 
> Honestly, I don't feel strongly enough to argue it.

I checked my devel config and I don't have DEBUG_VM enabled, while I
have a bunch of other debugging options related to locking or other
fine-grained sanity checks. The help text is not very specific what
exactly is being checked other that it hurts performance, so I read it
as that it's for MM developers that change the MM code, while in
filesystem we use the APIs.

However, for the this patchset I'll turn it on all testing instances of
course.

> Andrew?  David?  David this is going through your tree so would you feel more
> comfortable with 1 or the other?

I think it's a question for MM people, for now I assume it's supposed to
be VM_BUG_ON.

Patch
diff mbox series

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 0b5d89621cb9..520bbc67e67f 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -283,6 +283,7 @@  static inline void memcpy_page(struct page *dst_page, size_t dst_off,
 	char *dst = kmap_local_page(dst_page);
 	char *src = kmap_local_page(src_page);
 
+	BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
 	memcpy(dst + dst_off, src + src_off, len);
 	kunmap_local(src);
 	kunmap_local(dst);
@@ -295,6 +296,7 @@  static inline void memmove_page(struct page *dst_page, size_t dst_off,
 	char *dst = kmap_local_page(dst_page);
 	char *src = kmap_local_page(src_page);
 
+	BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
 	memmove(dst + dst_off, src + src_off, len);
 	kunmap_local(src);
 	kunmap_local(dst);
@@ -305,6 +307,7 @@  static inline void memset_page(struct page *page, size_t offset, int val,
 {
 	char *addr = kmap_local_page(page);
 
+	BUG_ON(offset + len > PAGE_SIZE);
 	memset(addr + offset, val, len);
 	kunmap_local(addr);
 }
@@ -314,6 +317,7 @@  static inline void memcpy_from_page(char *to, struct page *page,
 {
 	char *from = kmap_local_page(page);
 
+	BUG_ON(offset + len > PAGE_SIZE);
 	memcpy(to, from + offset, len);
 	kunmap_local(from);
 }
@@ -323,6 +327,7 @@  static inline void memcpy_to_page(struct page *page, size_t offset,
 {
 	char *to = kmap_local_page(page);
 
+	BUG_ON(offset + len > PAGE_SIZE);
 	memcpy(to + offset, from, len);
 	kunmap_local(to);
 }