[LTP] [PATCH 01/29] Hugetlb: Migrating libhugetlbfs brk_near_huge

[Tarun Sahu]

Hi, 

Thanks Cyril for reviewing it.
Please check my comments below.

On
Mon, 2022-10-17 at 11:30 +0200, Cyril Hrubis wrote:

-- skip
> + *
> > + * Test Name: brk near hugepage
> > + *
> > + * Test Description: Certain kernels have a bug where brk() does
> > not perform
> > + * the same checks that a MAP_FIXED mmap() will, allowing brk() to
> > create a
> > + * normal page VMA in a hugepage only address region. This can
> > lead to oopses
> > + * or other badness.
> 
> This description has to be in a separate ascii-doc formatted comment
> that starts with /*\

Ok. I will update this in all of the patches in this series.

-- skip
> > +	p = SAFE_MMAP(hugemap_addr, hpage_size, PROT_READ|PROT_WRITE,
> > +			MAP_PRIVATE|MAP_FIXED, fd, 0);
> > +	if (p != hugemap_addr) {
> > +		tst_res(TFAIL, "mmap() at unexpected address %p instead
> > of %p\n", p,
> > +		     hugemap_addr);
> > +		goto fail;
> 
> Can we just do return here instead. The failure has been reported
> there
> is no point in calling tst_brk(TBROK, ...) as well.
When we run one iteration only, tst_brk does not make sense, I agree.
But if we are running more than one iteration (i >= 2), test
should not continue to next iteration if the current iteration fails.
Only way I could find is to use tst_brk(TBROK,... , as tst_brk(TFAIl...
is deprecated.

> 
> > +	}
> > +
> > +	err = test_addr_huge((void *)p);
> > +	if (err != 1) {
> > +		tst_res(TFAIL, "Mapped address is not hugepage");
> > +		goto fail;
> > +	}
> 
> I do not get why we even need this check. We map a file located on
> hugetlbfs and then we stat it and check that it indeed is on
> hutetlbfs.
> What did we expect to fail here? I would say that the mmap() with
> right
> path is enough to make sure that the file is created on hugetlbfs.
> 
I agree, this check is redundant, I will remove it.

> > +	newbrk = next_chunk(brk0) + getpagesize();
> > +	err = brk((void *)newbrk);
> > +	if (err == -1) {
> > +		/* Failing the brk() is an acceptable kernel response
> > */
> > +		tst_res(TPASS, "Failing the brk is an acceptable
> > response");
> > +	} else {
> > +		/* Suceeding the brk() is acceptable iff the new memory
> > is
> > +		 * properly accesible and we don't have a kernel blow
> > up when
> > +		 * we touch it.
> > +		 */
> > +		memset(brk0, 0, newbrk-brk0);
> > +		tst_res(TPASS, "Succeding brk is acceptable, as memset
> > confirms that "
> > +				"new memory is properly accessible
> > without kernel blow up");
> > +	}
> > +	SAFE_MUNMAP(p, hpage_size);
> > +	SAFE_CLOSE(fd);
> 
> Shouldn't we brk() back to the original brk0? Does the test work with
> -i 2?
> 
Yes, will update it back to brk0.
Yes -i 2.. working, may be becuase, we
are just moving it by 1 page.

--skip
> > +
> > +static void cleanup(void)
> > +{
> > +	if (fd >= 0)
> > +		SAFE_CLOSE(fd);
> > +	umount2(Hopt, MNT_DETACH);
> 
> We whould umount here only if we actually have mounted something.
umount only, will require explicit unmap when test fails or break.
For
that, all the local variable for address mapping will have to be
static defined so that they can be accessed in cleanup() function.

I tried to avoid it by using umount2 which eventually umount when
process unmaps all the mappings after it finishes.