diff mbox series

unshare01.sh: Setup parent mount flag before unshare testing

Message ID 20210223140323.126555-1-zhaogongyi@huawei.com
State New
Headers show
Series unshare01.sh: Setup parent mount flag before unshare testing | expand

Commit Message

Zhao Gongyi Feb. 23, 2021, 2:03 p.m. UTC
We need setup parent mount flag to shared before unshare testing, or it will
fail for system which has no systemd service since the propagation flag is
changed by systemd. From man 7 mount_namespaces.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
 testcases/commands/unshare/unshare01.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--
2.17.1

Comments

Petr Vorel Feb. 24, 2021, 1:40 a.m. UTC | #1
Hi,

> We need setup parent mount flag to shared before unshare testing, or it will
> fail for system which has no systemd service since the propagation flag is
> changed by systemd. From man 7 mount_namespaces.
Do I understand correctly that all distros without systemd are affected,
because systemd "automatically remounts all mount points as MS_SHARED
on system startup" and test expect it?

> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
>  testcases/commands/unshare/unshare01.sh | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

> diff --git a/testcases/commands/unshare/unshare01.sh b/testcases/commands/unshare/unshare01.sh
> index bf163a7f4..e1fb15035 100755
> --- a/testcases/commands/unshare/unshare01.sh
> +++ b/testcases/commands/unshare/unshare01.sh
> @@ -31,7 +31,6 @@ TST_SETUP=setup
>  TST_CLEANUP=cleanup
>  TST_TESTFUNC=do_test
>  TST_NEEDS_ROOT=1
> -TST_NEEDS_TMPDIR=1
You still need TST_NEEDS_TMPDIR=1, because you create files and directories.

Also your patch breaks bind test on very old systems (kernel 2.6, util-linux
2.17.2, glibc 2.12):
unshare01 5 TFAIL: unshare --mount mount --bind dir_A dir_B got bind info

Any idea why (how to avoid this regression)?

>  TST_NEEDS_CMDS="unshare id mount umount"
>  . tst_test.sh

> @@ -39,6 +38,7 @@ max_userns_path="/proc/sys/user/max_user_namespaces"
>  max_mntns_path="/proc/sys/user/max_mnt_namespaces"
>  default_max_userns=-1
>  default_max_mntns=-1
> +CURR=$(pwd)
Instead of $CURR, cd - can be used.

>  setup()
>  {
> @@ -55,6 +55,10 @@ setup()
>  		echo 1024 > "${max_mntns_path}"
>  	fi

> +	mkdir $CURR/dir_C
just mkdir dir_C
> +	mount -t tmpfs none dir_C
> +	mount --make-shared dir_C
FYI We have tst_mount, but it'd not help much here.

> +	cd dir_C
>  	mkdir -p dir_A dir_B
>  	touch dir_A/A dir_B/B
>  }
> @@ -66,6 +70,9 @@ cleanup()
>  		echo ${default_max_userns} > "${max_userns_path}"
>  	[ ${default_max_mntns} -ne -1 ] && \
>  		echo ${default_max_mntns} > "${max_mntns_path}"
> +	cd $CURR
> +	umount dir_C
tst_umount dir_C

> +	rm -rf dir_C
rm is not needed (cleanup is done automatically).
>  }

>  check_id()

Full diff of changes I propose below.

Kind regards,
Petr

diff --git testcases/commands/unshare/unshare01.sh testcases/commands/unshare/unshare01.sh
index e1fb15035..0b5c56811 100755
--- testcases/commands/unshare/unshare01.sh
+++ testcases/commands/unshare/unshare01.sh
@@ -31,6 +31,7 @@ TST_SETUP=setup
 TST_CLEANUP=cleanup
 TST_TESTFUNC=do_test
 TST_NEEDS_ROOT=1
+TST_NEEDS_TMPDIR=1
 TST_NEEDS_CMDS="unshare id mount umount"
 . tst_test.sh
 
@@ -38,7 +39,6 @@ max_userns_path="/proc/sys/user/max_user_namespaces"
 max_mntns_path="/proc/sys/user/max_mnt_namespaces"
 default_max_userns=-1
 default_max_mntns=-1
-CURR=$(pwd)
 
 setup()
 {
@@ -55,7 +55,7 @@ setup()
 		echo 1024 > "${max_mntns_path}"
 	fi
 
-	mkdir $CURR/dir_C
+	mkdir dir_C
 	mount -t tmpfs none dir_C
 	mount --make-shared dir_C
 	cd dir_C
@@ -70,9 +70,8 @@ cleanup()
 		echo ${default_max_userns} > "${max_userns_path}"
 	[ ${default_max_mntns} -ne -1 ] && \
 		echo ${default_max_mntns} > "${max_mntns_path}"
-	cd $CURR
-	umount dir_C
-	rm -rf dir_C
+	cd - >/dev/null
+	tst_umount dir_C
 }
 
 check_id()
Xiao Yang Feb. 24, 2021, 5:01 a.m. UTC | #2
Hi Zhongyi, Petr

I don't like the approach which enforces mountpoint to be shared in 
parent mount namespace.
I think we can tune expected value by checking propagation flag in 
parent mount namespace because of two reasons:
1) Make test cover more cases.
2) Don't depend on the fixed tmpfs.

Zhongyi,  could you test the following patch on your enviorment?
-------------------------------------------------------------------------------------------------
diff --git a/testcases/commands/unshare/unshare01.sh 
b/testcases/commands/unshare/unshare01.sh
index bf163a7f4..78ea83fc0 100755
--- a/testcases/commands/unshare/unshare01.sh
+++ b/testcases/commands/unshare/unshare01.sh
@@ -40,6 +40,17 @@ max_mntns_path="/proc/sys/user/max_mnt_namespaces"
  default_max_userns=-1
  default_max_mntns=-1

+parse_propagation_flag()
+{
+       mount --bind dir_A dir_B
+       if grep -w 'dir_B' /proc/self/mountinfo | grep -qw 'shared'; then
+               echo "mounted"
+       else
+               echo "unmounted"
+       fi
+       umount dir_B
+}
+
  setup()
  {
         # On some distributions(e.g RHEL7.4), the default value of
@@ -149,7 +160,8 @@ do_test()
         4) unshare_test "--user --map-root-user" "id -g" "0";;
         5) unshare_test "--mount" "mount --bind dir_A dir_B" "unmounted";;
         6) unshare_test "--mount --propagation shared" \
-                       "mount --bind dir_A dir_B" "mounted";;
+                       "mount --bind dir_A dir_B" \
+                       "$(parse_propagation_flag)";;
         7) unshare_test "--user --map-root-user --mount" \
                         "mount --bind dir_A dir_B" "unmounted";;
         8) unshare_test "--user --map-root-user --mount --propagation 
shared" \
--
------------------------------------------------------------------------------------------

Best Regards,
Xiao Yang
On 2021/2/24 9:40, Petr Vorel wrote:
> Hi,
>
>> We need setup parent mount flag to shared before unshare testing, or it will
>> fail for system which has no systemd service since the propagation flag is
>> changed by systemd. From man 7 mount_namespaces.
> Do I understand correctly that all distros without systemd are affected,
> because systemd "automatically remounts all mount points as MS_SHARED
> on system startup" and test expect it?
>
>> Signed-off-by: Zhao Gongyi<zhaogongyi@huawei.com>
>> ---
>>   testcases/commands/unshare/unshare01.sh | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>> diff --git a/testcases/commands/unshare/unshare01.sh b/testcases/commands/unshare/unshare01.sh
>> index bf163a7f4..e1fb15035 100755
>> --- a/testcases/commands/unshare/unshare01.sh
>> +++ b/testcases/commands/unshare/unshare01.sh
>> @@ -31,7 +31,6 @@ TST_SETUP=setup
>>   TST_CLEANUP=cleanup
>>   TST_TESTFUNC=do_test
>>   TST_NEEDS_ROOT=1
>> -TST_NEEDS_TMPDIR=1
> You still need TST_NEEDS_TMPDIR=1, because you create files and directories.
>
> Also your patch breaks bind test on very old systems (kernel 2.6, util-linux
> 2.17.2, glibc 2.12):
> unshare01 5 TFAIL: unshare --mount mount --bind dir_A dir_B got bind info
>
> Any idea why (how to avoid this regression)?
>
>>   TST_NEEDS_CMDS="unshare id mount umount"
>>   . tst_test.sh
>> @@ -39,6 +38,7 @@ max_userns_path="/proc/sys/user/max_user_namespaces"
>>   max_mntns_path="/proc/sys/user/max_mnt_namespaces"
>>   default_max_userns=-1
>>   default_max_mntns=-1
>> +CURR=$(pwd)
> Instead of $CURR, cd - can be used.
>
>>   setup()
>>   {
>> @@ -55,6 +55,10 @@ setup()
>>   		echo 1024>  "${max_mntns_path}"
>>   	fi
>> +	mkdir $CURR/dir_C
> just mkdir dir_C
>> +	mount -t tmpfs none dir_C
>> +	mount --make-shared dir_C
> FYI We have tst_mount, but it'd not help much here.
>
>> +	cd dir_C
>>   	mkdir -p dir_A dir_B
>>   	touch dir_A/A dir_B/B
>>   }
>> @@ -66,6 +70,9 @@ cleanup()
>>   		echo ${default_max_userns}>  "${max_userns_path}"
>>   	[ ${default_max_mntns} -ne -1 ]&&  \
>>   		echo ${default_max_mntns}>  "${max_mntns_path}"
>> +	cd $CURR
>> +	umount dir_C
> tst_umount dir_C
>
>> +	rm -rf dir_C
> rm is not needed (cleanup is done automatically).
>>   }
>>   check_id()
> Full diff of changes I propose below.
>
> Kind regards,
> Petr
>
> diff --git testcases/commands/unshare/unshare01.sh testcases/commands/unshare/unshare01.sh
> index e1fb15035..0b5c56811 100755
> --- testcases/commands/unshare/unshare01.sh
> +++ testcases/commands/unshare/unshare01.sh
> @@ -31,6 +31,7 @@ TST_SETUP=setup
>   TST_CLEANUP=cleanup
>   TST_TESTFUNC=do_test
>   TST_NEEDS_ROOT=1
> +TST_NEEDS_TMPDIR=1
>   TST_NEEDS_CMDS="unshare id mount umount"
>   . tst_test.sh
>
> @@ -38,7 +39,6 @@ max_userns_path="/proc/sys/user/max_user_namespaces"
>   max_mntns_path="/proc/sys/user/max_mnt_namespaces"
>   default_max_userns=-1
>   default_max_mntns=-1
> -CURR=$(pwd)
>
>   setup()
>   {
> @@ -55,7 +55,7 @@ setup()
>   		echo 1024>  "${max_mntns_path}"
>   	fi
>
> -	mkdir $CURR/dir_C
> +	mkdir dir_C
>   	mount -t tmpfs none dir_C
>   	mount --make-shared dir_C
>   	cd dir_C
> @@ -70,9 +70,8 @@ cleanup()
>   		echo ${default_max_userns}>  "${max_userns_path}"
>   	[ ${default_max_mntns} -ne -1 ]&&  \
>   		echo ${default_max_mntns}>  "${max_mntns_path}"
> -	cd $CURR
> -	umount dir_C
> -	rm -rf dir_C
> +	cd ->/dev/null
> +	tst_umount dir_C
>   }
>
>   check_id()
>
diff mbox series

Patch

diff --git a/testcases/commands/unshare/unshare01.sh b/testcases/commands/unshare/unshare01.sh
index bf163a7f4..e1fb15035 100755
--- a/testcases/commands/unshare/unshare01.sh
+++ b/testcases/commands/unshare/unshare01.sh
@@ -31,7 +31,6 @@  TST_SETUP=setup
 TST_CLEANUP=cleanup
 TST_TESTFUNC=do_test
 TST_NEEDS_ROOT=1
-TST_NEEDS_TMPDIR=1
 TST_NEEDS_CMDS="unshare id mount umount"
 . tst_test.sh

@@ -39,6 +38,7 @@  max_userns_path="/proc/sys/user/max_user_namespaces"
 max_mntns_path="/proc/sys/user/max_mnt_namespaces"
 default_max_userns=-1
 default_max_mntns=-1
+CURR=$(pwd)

 setup()
 {
@@ -55,6 +55,10 @@  setup()
 		echo 1024 > "${max_mntns_path}"
 	fi

+	mkdir $CURR/dir_C
+	mount -t tmpfs none dir_C
+	mount --make-shared dir_C
+	cd dir_C
 	mkdir -p dir_A dir_B
 	touch dir_A/A dir_B/B
 }
@@ -66,6 +70,9 @@  cleanup()
 		echo ${default_max_userns} > "${max_userns_path}"
 	[ ${default_max_mntns} -ne -1 ] && \
 		echo ${default_max_mntns} > "${max_mntns_path}"
+	cd $CURR
+	umount dir_C
+	rm -rf dir_C
 }

 check_id()