| Message ID | 20210210062221.3023586-5-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: convert kmaps to core page calls | expand |
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);
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>
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>
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); > > >
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.
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
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.
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
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.
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); }