<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 6, 2020 at 9:53 AM Yang Xu <<a href="mailto:xuyang2018.jy@cn.fujitsu.com">xuyang2018.jy@cn.fujitsu.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Petr<br>
<br>
> c7a2d296b didn't reset stat_time, thus uninitialized set_shared was<br>
> assigned to test variable and test got value from a null pointer,<br>
> which leaded to segfault.<br>
> <br>
> hugeshmctl01.c:279: PASS: shmctl in func_rmid() failed as expected, shared memory appears to be removed<br>
> tst_checkpoint.c:147: BROK: hugeshmctl01.c:152: tst_checkpoint_wait(0, 10000): ETIMEDOUT (110)<br>
> mem.c:817: INFO: set nr_hugepages to 0<br>
> <br>
> dmesg:<br>
> segfault at 7f73f8c00000 ip 00000000004051e1 sp 00007ffef375f9a0 error 6 in hugeshmctl01.master[404000+13000]<br>
> addr2line -e hugeshmctl01 -f 00000000004051e1<br>
> stat_setup<br>
> hugeshmctl01.c:139 (discriminator 4)<br>
> <br>
> test = (stat_time == FIRST) ? set_shmat() : set_shared;<br>
> <br>
> /* do an assignement for fun */<br>
> *(int *)test = i; // error here<br>
> <br>
> Fixes: c7a2d296b ("hugeshmctl: Use loop from the API")<br>
> <br>
> Reported-by: Yang Xu <<a href="mailto:xuyang2018.jy@cn.fujitsu.com" target="_blank">xuyang2018.jy@cn.fujitsu.com</a>><br>
> Suggested-by: Yang Xu <<a href="mailto:xuyang2018.jy@cn.fujitsu.com" target="_blank">xuyang2018.jy@cn.fujitsu.com</a>><br>
> Signed-off-by: Petr Vorel <<a href="mailto:pvorel@suse.cz" target="_blank">pvorel@suse.cz</a>><br>
> ---<br>
> Hi Xu,<br>
> <br>
> I'm sorry for introducing a regression.<br>
> Thank you for a report and fixing the test.<br>
> I'd personally prefer to keep .tcnt = ARRAY_SIZE(tcases),<br>
> but maybe others will prefer to keep loop in test_hugeshmctl()<br>
> as it was before my change.<br>
> <br>
> BTW it'd be nice to have something like setup_i() and cleanup_i(),<br>
> which would be called before/after each run of whole test, when run with<br>
> -i n.<br>
Sub tests have own clean and setup function. They only reused a System <br>
V shared memory segment. IMO, this is a question of coupling.<br>
> <br>
> Kind regards,<br>
> Petr<br>
> <br>
> .../mem/hugetlb/hugeshmctl/hugeshmctl01.c | 27 ++++++++++---------<br>
> 1 file changed, 14 insertions(+), 13 deletions(-)<br>
> <br>
> diff --git a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c<br>
> index e6cf8bf09..3b7e14363 100644<br>
> --- a/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c<br>
> +++ b/testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c<br>
> @@ -75,6 +75,20 @@ struct tcase {<br>
> <br>
> static void test_hugeshmctl(unsigned int i)<br>
> {<br>
> + stat_time = FIRST;<br>
> +<br>
My description may confuse you. stat_time should not be reseted every <br>
time, it only needs to be reseted when next loop. This value will be +1 <br>
when call stat_cleanup.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Xu Yang is correct here. If we do reset 'stat_time' to FIRST in each loop then it would be missing the SECOND part in the stat_setup(). We can fix the problem simply to move the 'stat_time' to if-condition as he suggested.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">But to be honest, the looping workflow of hugeshmctl01 looks a little bit in disorder that may be the reason makes Petr missed the 'stat_time' value part, I appreciated if someone who can help to refactor the hugeshmctl01:).</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
struct tcase {<br>
int cmd;<br>
void (*func_test) (void);<br>
void (*func_setup) (void);<br>
} tcases[] = {<br>
{IPC_STAT, func_stat, stat_setup}, //stat_time = FIRST<br>
{IPC_STAT, func_stat, stat_setup}, //stat_time = SECOND<br>
<br>
As you do, the first and second case are same. it should be added into <br>
the "if == 0".<br>
<br>
ps: I personally think old case is more cleaner. Let's hear from others.<br>
<br>
Best Regards<br>
Yang Xu<br>
> + /*<br>
> + * Create a shared memory segment with read and write<br>
> + * permissions. Do this here instead of in setup()<br>
> + * so that looping (-i) will work correctly.<br>
> + */<br>
> + if (i == 0) {<br>
> + shm_id_1 = shmget(shmkey, shm_size,<br>
> + SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);<br>
> + if (shm_id_1 == -1)<br>
> + tst_brk(TBROK | TERRNO, "shmget #main");<br>
> + }<br>
> +<br>
> /*<br>
> * if needed, set up any required conditions by<br>
> * calling the appropriate setup function<br>
> @@ -296,19 +310,6 @@ void setup(void)<br>
> shm_size = hpage_size * hugepages / 2;<br>
> update_shm_size(&shm_size);<br>
> shmkey = getipckey();<br>
> -<br>
> - /* initialize stat_time */<br>
> - stat_time = FIRST;<br>
> -<br>
> - /*<br>
> - * Create a shared memory segment with read and write<br>
> - * permissions. Do this here instead of in setup()<br>
> - * so that looping (-i) will work correctly.<br>
> - */<br>
> - shm_id_1 = shmget(shmkey, shm_size,<br>
> - SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);<br>
> - if (shm_id_1 == -1)<br>
> - tst_brk(TBROK | TERRNO, "shmget #main");<br>
> }<br>
> <br>
> void cleanup(void)<br>
> <br>
<br>
<br>
<br>
-- <br>
Mailing list info: <a href="https://lists.linux.it/listinfo/ltp" rel="noreferrer" target="_blank">https://lists.linux.it/listinfo/ltp</a><br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div>Regards,<br></div><div>Li Wang<br></div></div></div></div>