Re: [PATCH V2 4/8] mm/highmem: Add VM_BUG_ON() to mem*_page() calls

[Ira Weiny]

On Wed, Feb 10, 2021 at 06:57:30AM +0000, Chaitanya Kulkarni wrote:
> On 2/9/21 22:25, ira.weiny@xxxxxxxxx wrote:
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > 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);
> 
> 
>