From: Ira Weiny <[email protected]>
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/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
Cc: Christoph Hellwig <[email protected]>
Suggested-by: Matthew Wilcox <[email protected]>
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
New for V2
---
include/linux/highmem.h | 5 +++++
1 file changed, 5 insertions(+)
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);
}
--
2.28.0.rc0.12.gb6a658bd00c9
On 2/9/21 22:25, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> 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, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> 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 <[email protected]>
On Wed, Feb 10, 2021 at 12:55:02PM +0000, Christoph Hellwig wrote:
> On Tue, Feb 09, 2021 at 10:22:17PM -0800, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > 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/[email protected]/
>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <[email protected]>
On Wed, Feb 10, 2021 at 06:57:30AM +0000, Chaitanya Kulkarni wrote:
> On 2/9/21 22:25, [email protected] wrote:
> > From: Ira Weiny <[email protected]>
> >
> > 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, [email protected] wrote:
> > > From: Ira Weiny <[email protected]>
> > >
> > > 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, [email protected] wrote:
> > > > From: Ira Weiny <[email protected]>
> > > >
> > > > 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
From: Ira Weiny <[email protected]>
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.[1][2]
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
Suggested-by: Matthew Wilcox <[email protected]>
Suggested-by: Andrew Morton <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
Changes from V2:
Actually, really do VM_BUG_ON... <sigh>
Clean up commit message
---
include/linux/highmem.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 0b5d89621cb9..44170f312ae7 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);
+ VM_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);
+ VM_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);
+ VM_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);
+ VM_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);
+ VM_BUG_ON(offset + len > PAGE_SIZE);
memcpy(to + offset, from, len);
kunmap_local(to);
}
--
2.28.0.rc0.12.gb6a658bd00c9
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.