[PATCH] generic: test record locks across execve

[PATCH] generic: test record locks across execve

[Xiong Zhou]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6110 bytes --]

According to fcntl(2) man page, record locks are preserved across
an execve(2). This is a regression case for this.

Signed-off-by: Xiong Zhou <xzhou@redhat.com>
---

Thanks Daniel for the report and reproducer!
Jeff Layton is fixing this in kernel upstream.

 .gitignore            |  1 +
 src/Makefile          |  2 +-
 src/t_locks_execve.c  | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/482     | 70 +++++++++++++++++++++++++++++++++++++++++
 tests/generic/482.out |  2 ++
 tests/generic/group   |  1 +
 6 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100644 src/t_locks_execve.c
 create mode 100755 tests/generic/482
 create mode 100644 tests/generic/482.out

diff --git a/.gitignore b/.gitignore
index 368d11c..192ca35 100644
--- a/.gitignore
+++ b/.gitignore
@@ -121,6 +121,7 @@
 /src/t_getcwd
 /src/t_holes
 /src/t_immutable
+/src/t_locks_execve
 /src/t_mmap_cow_race
 /src/t_mmap_dio
 /src/t_mmap_fallocate
diff --git a/src/Makefile b/src/Makefile
index 0d3feae..6ca5636 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -15,7 +15,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
 	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
 	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
-	t_ofd_locks
+	t_ofd_locks t_locks_execve
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
new file mode 100644
index 0000000..a93c6ca
--- /dev/null
+++ b/src/t_locks_execve.c
@@ -0,0 +1,86 @@
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include <stdio.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+static void err_exit(char *op, int errn)
+{
+	fprintf(stderr, "%s: %s\n", op, strerror(errn));
+	exit(errn);
+}
+
+void *thread_fn(void *arg)
+{
+	/* execve will release threads */
+	while(1) sleep(1);
+	return NULL;
+}
+
+static void checklock(int fd)
+{
+	pid_t child = fork();
+	if (child < 0)
+		err_exit("fork", errno);
+	if (child == 0) {
+		struct flock fl = {
+			.l_type = F_WRLCK,
+			.l_whence = SEEK_SET,
+			.l_start = 0,
+			.l_len = 1,
+		};
+
+		if (fcntl(fd, F_GETLK, &fl) < 0)
+			err_exit("getlk", errno);
+
+		if (fl.l_type == F_UNLCK)
+			printf("Lock released\n");
+		exit(0);
+	}
+	waitpid(child, NULL, 0);
+}
+
+int main(int argc, char **argv) {
+
+	int fd;
+	/* passing fd in argv[2] in execve */
+	if (argc == 3) {
+		fd = atoi(argv[2]);
+		checklock(fd);
+		exit(0);
+	}
+
+	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
+	if (fd < 0)
+		err_exit("open", errno);
+	struct flock fl = {
+		.l_type = F_WRLCK,
+		.l_whence = SEEK_SET,
+		.l_start = 0,
+		.l_len = 1,
+	};
+	if (fcntl(fd, F_SETLK, &fl) < 0)
+		err_exit("setlk", errno);
+
+	pthread_t th;
+	pthread_create(&th, NULL, thread_fn, &fd);
+
+	int flags;
+	if ((flags = fcntl(fd, F_GETFD)) < 0)
+		err_exit("getfd", errno);
+	flags &= ~FD_CLOEXEC;
+	if (fcntl(fd, F_SETFD, flags) < 0)
+		err_exit("setfd", errno);
+
+	char fdstr[10];
+	snprintf(fdstr, sizeof(fdstr), "%d", fd);
+	char *newargv[] = { argv[0], argv[1], fdstr, NULL };
+	execve(argv[0], newargv, NULL);
+	return 0;
+}
diff --git a/tests/generic/482 b/tests/generic/482
new file mode 100755
index 0000000..f7cb251
--- /dev/null
+++ b/tests/generic/482
@@ -0,0 +1,70 @@
+#! /bin/bash
+# FS QA Test 482
+#
+# This is testing
+#
+#  "Record locks are not inherited by a child created via fork(2),
+#   but are preserved across an execve(2)."
+#
+# from fcntl(2) man page.
+#
+# Author: "Daniel P. Berrangé" <berrange@redhat.com>
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2018 RedHat Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_test
+
+# prepare a 4k testfile in TEST_DIR
+$XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \
+	$TEST_DIR/t_lock_execve_file >> $seqres.full 2>&1
+
+# run test programe
+$here/src/t_locks_execve $TEST_DIR/t_lock_execve_file
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/482.out b/tests/generic/482.out
new file mode 100644
index 0000000..f5e17c8
--- /dev/null
+++ b/tests/generic/482.out
@@ -0,0 +1,2 @@
+QA output created by 482
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index e867606..0bf3c1f 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -484,3 +484,4 @@
 479 auto quick metadata
 480 auto quick metadata
 481 auto quick log metadata
+482 auto lock quick
-- 
1.8.3.1

Re: [PATCH] generic: test record locks across execve

[Eryu Guan]

On Tue, Apr 03, 2018 at 10:03:45PM +0800, Xiong Zhou wrote:
> According to fcntl(2) man page, record locks are preserved across
> an execve(2). This is a regression case for this.
> 
> Signed-off-by: Xiong Zhou <xzhou@redhat.com>

Sorry for the late review..

> ---
> 
> Thanks Daniel for the report and reproducer!
> Jeff Layton is fixing this in kernel upstream.

If it's already fixed, can you please reference the commit id in the
log?

> 
>  .gitignore            |  1 +
>  src/Makefile          |  2 +-
>  src/t_locks_execve.c  | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/482     | 70 +++++++++++++++++++++++++++++++++++++++++
>  tests/generic/482.out |  2 ++
>  tests/generic/group   |  1 +
>  6 files changed, 161 insertions(+), 1 deletion(-)
>  create mode 100644 src/t_locks_execve.c
>  create mode 100755 tests/generic/482
>  create mode 100644 tests/generic/482.out
> 
> diff --git a/.gitignore b/.gitignore
> index 368d11c..192ca35 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -121,6 +121,7 @@
>  /src/t_getcwd
>  /src/t_holes
>  /src/t_immutable
> +/src/t_locks_execve
>  /src/t_mmap_cow_race
>  /src/t_mmap_dio
>  /src/t_mmap_fallocate
> diff --git a/src/Makefile b/src/Makefile
> index 0d3feae..6ca5636 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -15,7 +15,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>  	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
>  	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
>  	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
> -	t_ofd_locks
> +	t_ofd_locks t_locks_execve
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
> new file mode 100644
> index 0000000..a93c6ca
> --- /dev/null
> +++ b/src/t_locks_execve.c
> @@ -0,0 +1,86 @@
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
> +
> +static void err_exit(char *op, int errn)
> +{
> +	fprintf(stderr, "%s: %s\n", op, strerror(errn));
> +	exit(errn);
> +}
> +
> +void *thread_fn(void *arg)
> +{
> +	/* execve will release threads */
> +	while(1) sleep(1);
> +	return NULL;
> +}
> +
> +static void checklock(int fd)
> +{
> +	pid_t child = fork();
> +	if (child < 0)
> +		err_exit("fork", errno);
> +	if (child == 0) {
> +		struct flock fl = {
> +			.l_type = F_WRLCK,
> +			.l_whence = SEEK_SET,
> +			.l_start = 0,
> +			.l_len = 1,
> +		};

The struct flock used here is the same as in main(), make it a global
struct?

> +
> +		if (fcntl(fd, F_GETLK, &fl) < 0)
> +			err_exit("getlk", errno);
> +
> +		if (fl.l_type == F_UNLCK)
> +			printf("Lock released\n");

I'd like to see more descriptive error message here.

> +		exit(0);

exit with non-zero on failure?

> +	}
> +	waitpid(child, NULL, 0);
> +}
> +
> +int main(int argc, char **argv) {

int main(...)
{
...
}

> +
> +	int fd;
> +	/* passing fd in argv[2] in execve */
> +	if (argc == 3) {
> +		fd = atoi(argv[2]);
> +		checklock(fd);
> +		exit(0);
> +	}
> +
> +	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
> +	if (fd < 0)
> +		err_exit("open", errno);
> +	struct flock fl = {
> +		.l_type = F_WRLCK,
> +		.l_whence = SEEK_SET,
> +		.l_start = 0,
> +		.l_len = 1,
> +	};
> +	if (fcntl(fd, F_SETLK, &fl) < 0)
> +		err_exit("setlk", errno);
> +
> +	pthread_t th;
> +	pthread_create(&th, NULL, thread_fn, &fd);

I'm not sure why we need a thread here. Add more comments?

> +
> +	int flags;
> +	if ((flags = fcntl(fd, F_GETFD)) < 0)
> +		err_exit("getfd", errno);
> +	flags &= ~FD_CLOEXEC;
> +	if (fcntl(fd, F_SETFD, flags) < 0)
> +		err_exit("setfd", errno);
> +
> +	char fdstr[10];
> +	snprintf(fdstr, sizeof(fdstr), "%d", fd);
> +	char *newargv[] = { argv[0], argv[1], fdstr, NULL };

Better to declare all the variables at the beginning of main.

> +	execve(argv[0], newargv, NULL);
> +	return 0;
> +}
> diff --git a/tests/generic/482 b/tests/generic/482
> new file mode 100755
> index 0000000..f7cb251
> --- /dev/null
> +++ b/tests/generic/482
> @@ -0,0 +1,70 @@
> +#! /bin/bash
> +# FS QA Test 482
> +#
> +# This is testing
> +#
> +#  "Record locks are not inherited by a child created via fork(2),
> +#   but are preserved across an execve(2)."
> +#
> +# from fcntl(2) man page.
> +#
> +# Author: "Daniel P. Berrangé" <berrange@redhat.com>
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2018 RedHat Inc.  All Rights Reserved.

"Red Hat"?

> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_test
> +
> +# prepare a 4k testfile in TEST_DIR
> +$XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \
> +	$TEST_DIR/t_lock_execve_file >> $seqres.full 2>&1
> +
> +# run test programe
> +$here/src/t_locks_execve $TEST_DIR/t_lock_execve_file
> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/482.out b/tests/generic/482.out
> new file mode 100644
> index 0000000..f5e17c8
> --- /dev/null
> +++ b/tests/generic/482.out
> @@ -0,0 +1,2 @@
> +QA output created by 482
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index e867606..0bf3c1f 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -484,3 +484,4 @@
>  479 auto quick metadata
>  480 auto quick metadata
>  481 auto quick log metadata
> +482 auto lock quick

Please re-number it when resending :)

Thanks,
Eryu

> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] generic: test record locks across execve

[Xiong Zhou]

According to fcntl(2) man page, record locks are preserved across
an execve(2). This is a regression case for this.

Signed-off-by: Xiong Zhou <xzhou@redhat.com>
---
v2:
  Do not fork when checking lock after execve;
  More comments and some minor fixes.

Thanks Eryu for the review!
Jeff Layton's patch for this issue has not landed Linus tree.

 .gitignore            |  1 +
 src/Makefile          |  2 +-
 src/t_locks_execve.c  | 73 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/484     | 73 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/484.out |  2 ++
 tests/generic/group   |  1 +
 6 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 src/t_locks_execve.c
 create mode 100755 tests/generic/484
 create mode 100644 tests/generic/484.out

diff --git a/.gitignore b/.gitignore
index 368d11c8..192ca35e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -121,6 +121,7 @@
 /src/t_getcwd
 /src/t_holes
 /src/t_immutable
+/src/t_locks_execve
 /src/t_mmap_cow_race
 /src/t_mmap_dio
 /src/t_mmap_fallocate
diff --git a/src/Makefile b/src/Makefile
index 0d3feae1..6ca56366 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -15,7 +15,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
 	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
 	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
-	t_ofd_locks
+	t_ofd_locks t_locks_execve
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
new file mode 100644
index 00000000..dae4506d
--- /dev/null
+++ b/src/t_locks_execve.c
@@ -0,0 +1,73 @@
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include <stdio.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+static void err_exit(char *op, int errn)
+{
+	fprintf(stderr, "%s: %s\n", op, strerror(errn));
+	exit(errn);
+}
+
+void *thread_fn(void *arg)
+{
+	/* execve will release threads */
+	while(1) sleep(1);
+	return NULL;
+}
+
+struct flock fl = {
+	.l_type = F_WRLCK,
+	.l_whence = SEEK_SET,
+	.l_start = 0,
+	.l_len = 1,
+};
+
+static void checklock(int fd)
+{
+	if (fcntl(fd, F_GETLK, &fl) < 0)
+		err_exit("getlk", errno);
+	if (fl.l_type == F_UNLCK) {
+		printf("The write lock got released during execve\n");
+		exit(1);
+	}
+	exit(0);
+}
+
+int main(int argc, char **argv)
+{
+	int fd, flags;
+	char fdstr[10];
+	char *newargv[] = { argv[0], argv[1], fdstr, NULL };
+	/* passing fd in argv[2] in execve */
+	if (argc == 3) {
+		fd = atoi(argv[2]);
+		checklock(fd);
+		exit(0);
+	}
+	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
+	if (fd < 0)
+		err_exit("open", errno);
+	if (fcntl(fd, F_SETLK, &fl) < 0)
+		err_exit("setlk", errno);
+
+	/* spawning thread is necessary to reproduce the issue */
+	pthread_t th;
+	pthread_create(&th, NULL, thread_fn, &fd);
+
+	if ((flags = fcntl(fd, F_GETFD)) < 0)
+		err_exit("getfd", errno);
+	flags &= ~FD_CLOEXEC;
+	if (fcntl(fd, F_SETFD, flags) < 0)
+		err_exit("setfd", errno);
+	snprintf(fdstr, sizeof(fdstr), "%d", fd);
+	execve(argv[0], newargv, NULL);
+	return 0;
+}
diff --git a/tests/generic/484 b/tests/generic/484
new file mode 100755
index 00000000..76ebf1fd
--- /dev/null
+++ b/tests/generic/484
@@ -0,0 +1,73 @@
+#! /bin/bash
+# FS QA Test 484
+#
+# This is testing
+#
+#  "Record locks are not inherited by a child created via fork(2),
+#   but are preserved across an execve(2)."
+#
+# from fcntl(2) man page.
+#
+# Fixed by patch from Jeff Layton:
+# locks: change POSIX lock ownership on execve when files_struct is displaced
+#
+# Author: "Daniel P. Berrangé" <berrange@redhat.com>
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2018 Red Hat Inc.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_test
+
+# prepare a 4k testfile in TEST_DIR
+$XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \
+	$TEST_DIR/t_lock_execve_file >> $seqres.full 2>&1
+
+# run test programe
+$here/src/t_locks_execve $TEST_DIR/t_lock_execve_file
+
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/484.out b/tests/generic/484.out
new file mode 100644
index 00000000..94f2f0bd
--- /dev/null
+++ b/tests/generic/484.out
@@ -0,0 +1,2 @@
+QA output created by 484
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index ea8e51b3..798321f9 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -486,3 +486,4 @@
 481 auto quick log metadata
 482 auto metadata replay
 483 auto quick log metadata
+484 auto lock quick
-- 
2.17.0.252.gfe0a9ea

Re: [PATCH v2] generic: test record locks across execve

[Eryu Guan]

On Mon, Apr 23, 2018 at 10:42:48AM +0800, Xiong Zhou wrote:
> According to fcntl(2) man page, record locks are preserved across
> an execve(2). This is a regression case for this.

After looking at the kernel patch, this commit log doesn't look accurate
to me. The bug only happens if the process that calls execve(2) is a
multithread process. And this case only excrises this particular case.

Please put more background information in commit log and the test
description, this helps reviewers to understand the problem you're going
to test. I find that the commit log from Jeff Layton's patch seems good
enough:

"
POSIX mandates that open fds and their associated file locks should be
preserved across an execve. This works, unless the process is
multithreaded at the time that execve is called.

In that case, we'll end up unsharing the files_struct but the locks will
still have their fl_owner set to the address of the old one. Eventually,
when the other threads die and the last reference to the old
files_struct is put, any POSIX locks get torn down since it looks like
a close occurred on them.

The result is that all of your open files will be intact with none of
the locks you held before execve.
"

This also answers my previous question: why do we need an idle thread,
IMHO, "spawning thread is necessary to reproduce the issue" doesn't
really answer the question. But with above description it's fine to have
such comments in the code.

> 
> Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> ---
> v2:
>   Do not fork when checking lock after execve;
>   More comments and some minor fixes.
> 
> Thanks Eryu for the review!
> Jeff Layton's patch for this issue has not landed Linus tree.
> 
>  .gitignore            |  1 +
>  src/Makefile          |  2 +-
>  src/t_locks_execve.c  | 73 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/484     | 73 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/484.out |  2 ++
>  tests/generic/group   |  1 +
>  6 files changed, 151 insertions(+), 1 deletion(-)
>  create mode 100644 src/t_locks_execve.c
>  create mode 100755 tests/generic/484
>  create mode 100644 tests/generic/484.out
> 
> diff --git a/.gitignore b/.gitignore
> index 368d11c8..192ca35e 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -121,6 +121,7 @@
>  /src/t_getcwd
>  /src/t_holes
>  /src/t_immutable
> +/src/t_locks_execve
>  /src/t_mmap_cow_race
>  /src/t_mmap_dio
>  /src/t_mmap_fallocate
> diff --git a/src/Makefile b/src/Makefile
> index 0d3feae1..6ca56366 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -15,7 +15,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>  	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
>  	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
>  	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
> -	t_ofd_locks
> +	t_ofd_locks t_locks_execve
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
> new file mode 100644
> index 00000000..dae4506d
> --- /dev/null
> +++ b/src/t_locks_execve.c
> @@ -0,0 +1,73 @@
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
> +
> +static void err_exit(char *op, int errn)
> +{
> +	fprintf(stderr, "%s: %s\n", op, strerror(errn));
> +	exit(errn);
> +}
> +
> +void *thread_fn(void *arg)
> +{
> +	/* execve will release threads */
> +	while(1) sleep(1);
> +	return NULL;
> +}
> +
> +struct flock fl = {
> +	.l_type = F_WRLCK,
> +	.l_whence = SEEK_SET,
> +	.l_start = 0,
> +	.l_len = 1,
> +};
> +
> +static void checklock(int fd)
> +{
> +	if (fcntl(fd, F_GETLK, &fl) < 0)
> +		err_exit("getlk", errno);
> +	if (fl.l_type == F_UNLCK) {
> +		printf("The write lock got released during execve\n");
> +		exit(1);
> +	}
> +	exit(0);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int fd, flags;
> +	char fdstr[10];
> +	char *newargv[] = { argv[0], argv[1], fdstr, NULL };

We could add new line to make the code more readable, e.g. right here.

> +	/* passing fd in argv[2] in execve */
> +	if (argc == 3) {
> +		fd = atoi(argv[2]);
> +		checklock(fd);
> +		exit(0);
> +	}

And here :)

> +	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
> +	if (fd < 0)
> +		err_exit("open", errno);
> +	if (fcntl(fd, F_SETLK, &fl) < 0)
> +		err_exit("setlk", errno);
> +
> +	/* spawning thread is necessary to reproduce the issue */
> +	pthread_t th;
> +	pthread_create(&th, NULL, thread_fn, &fd);
> +
> +	if ((flags = fcntl(fd, F_GETFD)) < 0)
> +		err_exit("getfd", errno);
> +	flags &= ~FD_CLOEXEC;
> +	if (fcntl(fd, F_SETFD, flags) < 0)
> +		err_exit("setfd", errno);
> +	snprintf(fdstr, sizeof(fdstr), "%d", fd);
> +	execve(argv[0], newargv, NULL);
> +	return 0;
> +}
> diff --git a/tests/generic/484 b/tests/generic/484
> new file mode 100755
> index 00000000..76ebf1fd
> --- /dev/null
> +++ b/tests/generic/484
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# FS QA Test 484
> +#
> +# This is testing
> +#
> +#  "Record locks are not inherited by a child created via fork(2),
> +#   but are preserved across an execve(2)."
> +#
> +# from fcntl(2) man page.
> +#
> +# Fixed by patch from Jeff Layton:
> +# locks: change POSIX lock ownership on execve when files_struct is displaced
> +#
> +# Author: "Daniel P. Berrangé" <berrange@redhat.com>
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2018 Red Hat Inc.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_test
> +
> +# prepare a 4k testfile in TEST_DIR
> +$XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \
> +	$TEST_DIR/t_lock_execve_file >> $seqres.full 2>&1
> +
> +# run test programe

Unnecessary comment.

> +$here/src/t_locks_execve $TEST_DIR/t_lock_execve_file

I can fix the commit log and test description and the code style issue
on commit, no need to resend.

Thanks,
Eryu

> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/484.out b/tests/generic/484.out
> new file mode 100644
> index 00000000..94f2f0bd
> --- /dev/null
> +++ b/tests/generic/484.out
> @@ -0,0 +1,2 @@
> +QA output created by 484
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index ea8e51b3..798321f9 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -486,3 +486,4 @@
>  481 auto quick log metadata
>  482 auto metadata replay
>  483 auto quick log metadata
> +484 auto lock quick
> -- 
> 2.17.0.252.gfe0a9ea
> 

Re: [PATCH v2] generic: test record locks across execve

[Xiong Murphy Zhou]

On Fri, Apr 27, 2018 at 05:43:16PM +0800, Eryu Guan wrote:
> On Mon, Apr 23, 2018 at 10:42:48AM +0800, Xiong Zhou wrote:
> > According to fcntl(2) man page, record locks are preserved across
> > an execve(2). This is a regression case for this.
> 
> After looking at the kernel patch, this commit log doesn't look accurate
> to me. The bug only happens if the process that calls execve(2) is a
> multithread process. And this case only excrises this particular case.
> 
> Please put more background information in commit log and the test
> description, this helps reviewers to understand the problem you're going
> to test. I find that the commit log from Jeff Layton's patch seems good
> enough:
> 
> "
> POSIX mandates that open fds and their associated file locks should be
> preserved across an execve. This works, unless the process is
> multithreaded at the time that execve is called.
> 
> In that case, we'll end up unsharing the files_struct but the locks will
> still have their fl_owner set to the address of the old one. Eventually,
> when the other threads die and the last reference to the old
> files_struct is put, any POSIX locks get torn down since it looks like
> a close occurred on them.
> 
> The result is that all of your open files will be intact with none of
> the locks you held before execve.
> "
> 
> This also answers my previous question: why do we need an idle thread,
> IMHO, "spawning thread is necessary to reproduce the issue" doesn't
> really answer the question. But with above description it's fine to have
> such comments in the code.

Thank you! Lesson learned :)

I was too lazy to get into details. Thumb up for you!

Thanks,
Xiong

> 
> > 
> > Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> > ---
> > v2:
> >   Do not fork when checking lock after execve;
> >   More comments and some minor fixes.
> > 
> > Thanks Eryu for the review!
> > Jeff Layton's patch for this issue has not landed Linus tree.
> > 
> >  .gitignore            |  1 +
> >  src/Makefile          |  2 +-
> >  src/t_locks_execve.c  | 73 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/484     | 73 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/484.out |  2 ++
> >  tests/generic/group   |  1 +
> >  6 files changed, 151 insertions(+), 1 deletion(-)
> >  create mode 100644 src/t_locks_execve.c
> >  create mode 100755 tests/generic/484
> >  create mode 100644 tests/generic/484.out
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 368d11c8..192ca35e 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -121,6 +121,7 @@
> >  /src/t_getcwd
> >  /src/t_holes
> >  /src/t_immutable
> > +/src/t_locks_execve
> >  /src/t_mmap_cow_race
> >  /src/t_mmap_dio
> >  /src/t_mmap_fallocate
> > diff --git a/src/Makefile b/src/Makefile
> > index 0d3feae1..6ca56366 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -15,7 +15,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> >  	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
> >  	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
> >  	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
> > -	t_ofd_locks
> > +	t_ofd_locks t_locks_execve
> >  
> >  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> >  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> > diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
> > new file mode 100644
> > index 00000000..dae4506d
> > --- /dev/null
> > +++ b/src/t_locks_execve.c
> > @@ -0,0 +1,73 @@
> > +#ifndef _GNU_SOURCE
> > +#define _GNU_SOURCE
> > +#endif
> > +#include <stdio.h>
> > +#include <fcntl.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <pthread.h>
> > +#include <unistd.h>
> > +#include <sys/wait.h>
> > +
> > +static void err_exit(char *op, int errn)
> > +{
> > +	fprintf(stderr, "%s: %s\n", op, strerror(errn));
> > +	exit(errn);
> > +}
> > +
> > +void *thread_fn(void *arg)
> > +{
> > +	/* execve will release threads */
> > +	while(1) sleep(1);
> > +	return NULL;
> > +}
> > +
> > +struct flock fl = {
> > +	.l_type = F_WRLCK,
> > +	.l_whence = SEEK_SET,
> > +	.l_start = 0,
> > +	.l_len = 1,
> > +};
> > +
> > +static void checklock(int fd)
> > +{
> > +	if (fcntl(fd, F_GETLK, &fl) < 0)
> > +		err_exit("getlk", errno);
> > +	if (fl.l_type == F_UNLCK) {
> > +		printf("The write lock got released during execve\n");
> > +		exit(1);
> > +	}
> > +	exit(0);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	int fd, flags;
> > +	char fdstr[10];
> > +	char *newargv[] = { argv[0], argv[1], fdstr, NULL };
> 
> We could add new line to make the code more readable, e.g. right here.
> 
> > +	/* passing fd in argv[2] in execve */
> > +	if (argc == 3) {
> > +		fd = atoi(argv[2]);
> > +		checklock(fd);
> > +		exit(0);
> > +	}
> 
> And here :)
> 
> > +	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
> > +	if (fd < 0)
> > +		err_exit("open", errno);
> > +	if (fcntl(fd, F_SETLK, &fl) < 0)
> > +		err_exit("setlk", errno);
> > +
> > +	/* spawning thread is necessary to reproduce the issue */
> > +	pthread_t th;
> > +	pthread_create(&th, NULL, thread_fn, &fd);
> > +
> > +	if ((flags = fcntl(fd, F_GETFD)) < 0)
> > +		err_exit("getfd", errno);
> > +	flags &= ~FD_CLOEXEC;
> > +	if (fcntl(fd, F_SETFD, flags) < 0)
> > +		err_exit("setfd", errno);
> > +	snprintf(fdstr, sizeof(fdstr), "%d", fd);
> > +	execve(argv[0], newargv, NULL);
> > +	return 0;
> > +}
> > diff --git a/tests/generic/484 b/tests/generic/484
> > new file mode 100755
> > index 00000000..76ebf1fd
> > --- /dev/null
> > +++ b/tests/generic/484
> > @@ -0,0 +1,73 @@
> > +#! /bin/bash
> > +# FS QA Test 484
> > +#
> > +# This is testing
> > +#
> > +#  "Record locks are not inherited by a child created via fork(2),
> > +#   but are preserved across an execve(2)."
> > +#
> > +# from fcntl(2) man page.
> > +#
> > +# Fixed by patch from Jeff Layton:
> > +# locks: change POSIX lock ownership on execve when files_struct is displaced
> > +#
> > +# Author: "Daniel P. Berrangé" <berrange@redhat.com>
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2018 Red Hat Inc.  All Rights Reserved.
> > +#
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation.
> > +#
> > +# This program is distributed in the hope that it would be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write the Free Software Foundation,
> > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > +#-----------------------------------------------------------------------
> > +#
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_test
> > +
> > +# prepare a 4k testfile in TEST_DIR
> > +$XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \
> > +	$TEST_DIR/t_lock_execve_file >> $seqres.full 2>&1
> > +
> > +# run test programe
> 
> Unnecessary comment.
> 
> > +$here/src/t_locks_execve $TEST_DIR/t_lock_execve_file
> 
> I can fix the commit log and test description and the code style issue
> on commit, no need to resend.
> 
> Thanks,
> Eryu
> 
> > +
> > +# success, all done
> > +echo "Silence is golden"
> > +status=0
> > +exit
> > diff --git a/tests/generic/484.out b/tests/generic/484.out
> > new file mode 100644
> > index 00000000..94f2f0bd
> > --- /dev/null
> > +++ b/tests/generic/484.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 484
> > +Silence is golden
> > diff --git a/tests/generic/group b/tests/generic/group
> > index ea8e51b3..798321f9 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -486,3 +486,4 @@
> >  481 auto quick log metadata
> >  482 auto metadata replay
> >  483 auto quick log metadata
> > +484 auto lock quick
> > -- 
> > 2.17.0.252.gfe0a9ea
> > 

Re: [PATCH v2] generic: test record locks across execve

[Jeff Layton]

On Sat, 2018-04-28 at 07:28 +0800, Xiong Murphy Zhou wrote:
> On Fri, Apr 27, 2018 at 05:43:16PM +0800, Eryu Guan wrote:
> > On Mon, Apr 23, 2018 at 10:42:48AM +0800, Xiong Zhou wrote:
> > > According to fcntl(2) man page, record locks are preserved across
> > > an execve(2). This is a regression case for this.
> > 
> > After looking at the kernel patch, this commit log doesn't look accurate
> > to me. The bug only happens if the process that calls execve(2) is a
> > multithread process. And this case only excrises this particular case.
> > 
> > Please put more background information in commit log and the test
> > description, this helps reviewers to understand the problem you're going
> > to test. I find that the commit log from Jeff Layton's patch seems good
> > enough:
> > 
> > "
> > POSIX mandates that open fds and their associated file locks should be
> > preserved across an execve. This works, unless the process is
> > multithreaded at the time that execve is called.
> > 
> > In that case, we'll end up unsharing the files_struct but the locks will
> > still have their fl_owner set to the address of the old one. Eventually,
> > when the other threads die and the last reference to the old
> > files_struct is put, any POSIX locks get torn down since it looks like
> > a close occurred on them.
> > 
> > The result is that all of your open files will be intact with none of
> > the locks you held before execve.
> > "
> > 
> > This also answers my previous question: why do we need an idle thread,
> > IMHO, "spawning thread is necessary to reproduce the issue" doesn't
> > really answer the question. But with above description it's fine to have
> > such comments in the code.
> 
> Thank you! Lesson learned :)
> 
> I was too lazy to get into details. Thumb up for you!
> 
> Thanks,
> Xiong
> 

Thanks for adding the test.

Just to be clear, the patch I had originally proposed for this is not
going to work across all filesystems. We'll probably have to fix this in
a different way, but it's not clear yet what the fix should look like.

> > 
> > > 
> > > Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> > > ---
> > > v2:
> > >   Do not fork when checking lock after execve;
> > >   More comments and some minor fixes.
> > > 
> > > Thanks Eryu for the review!
> > > Jeff Layton's patch for this issue has not landed Linus tree.
> > > 
> > >  .gitignore            |  1 +
> > >  src/Makefile          |  2 +-
> > >  src/t_locks_execve.c  | 73 +++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/484     | 73 +++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/484.out |  2 ++
> > >  tests/generic/group   |  1 +
> > >  6 files changed, 151 insertions(+), 1 deletion(-)
> > >  create mode 100644 src/t_locks_execve.c
> > >  create mode 100755 tests/generic/484
> > >  create mode 100644 tests/generic/484.out
> > > 
> > > diff --git a/.gitignore b/.gitignore
> > > index 368d11c8..192ca35e 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -121,6 +121,7 @@
> > >  /src/t_getcwd
> > >  /src/t_holes
> > >  /src/t_immutable
> > > +/src/t_locks_execve
> > >  /src/t_mmap_cow_race
> > >  /src/t_mmap_dio
> > >  /src/t_mmap_fallocate
> > > diff --git a/src/Makefile b/src/Makefile
> > > index 0d3feae1..6ca56366 100644
> > > --- a/src/Makefile
> > > +++ b/src/Makefile
> > > @@ -15,7 +15,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> > >  	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
> > >  	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
> > >  	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
> > > -	t_ofd_locks
> > > +	t_ofd_locks t_locks_execve
> > >  
> > >  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> > >  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> > > diff --git a/src/t_locks_execve.c b/src/t_locks_execve.c
> > > new file mode 100644
> > > index 00000000..dae4506d
> > > --- /dev/null
> > > +++ b/src/t_locks_execve.c
> > > @@ -0,0 +1,73 @@
> > > +#ifndef _GNU_SOURCE
> > > +#define _GNU_SOURCE
> > > +#endif
> > > +#include <stdio.h>
> > > +#include <fcntl.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <errno.h>
> > > +#include <pthread.h>
> > > +#include <unistd.h>
> > > +#include <sys/wait.h>
> > > +
> > > +static void err_exit(char *op, int errn)
> > > +{
> > > +	fprintf(stderr, "%s: %s\n", op, strerror(errn));
> > > +	exit(errn);
> > > +}
> > > +
> > > +void *thread_fn(void *arg)
> > > +{
> > > +	/* execve will release threads */
> > > +	while(1) sleep(1);
> > > +	return NULL;
> > > +}
> > > +
> > > +struct flock fl = {
> > > +	.l_type = F_WRLCK,
> > > +	.l_whence = SEEK_SET,
> > > +	.l_start = 0,
> > > +	.l_len = 1,
> > > +};
> > > +
> > > +static void checklock(int fd)
> > > +{
> > > +	if (fcntl(fd, F_GETLK, &fl) < 0)
> > > +		err_exit("getlk", errno);
> > > +	if (fl.l_type == F_UNLCK) {
> > > +		printf("The write lock got released during execve\n");
> > > +		exit(1);
> > > +	}
> > > +	exit(0);
> > > +}
> > > +
> > > +int main(int argc, char **argv)
> > > +{
> > > +	int fd, flags;
> > > +	char fdstr[10];
> > > +	char *newargv[] = { argv[0], argv[1], fdstr, NULL };
> > 
> > We could add new line to make the code more readable, e.g. right here.
> > 
> > > +	/* passing fd in argv[2] in execve */
> > > +	if (argc == 3) {
> > > +		fd = atoi(argv[2]);
> > > +		checklock(fd);
> > > +		exit(0);
> > > +	}
> > 
> > And here :)
> > 
> > > +	fd = open(argv[1], O_WRONLY|O_CREAT, 0755);
> > > +	if (fd < 0)
> > > +		err_exit("open", errno);
> > > +	if (fcntl(fd, F_SETLK, &fl) < 0)
> > > +		err_exit("setlk", errno);
> > > +
> > > +	/* spawning thread is necessary to reproduce the issue */
> > > +	pthread_t th;
> > > +	pthread_create(&th, NULL, thread_fn, &fd);
> > > +
> > > +	if ((flags = fcntl(fd, F_GETFD)) < 0)
> > > +		err_exit("getfd", errno);
> > > +	flags &= ~FD_CLOEXEC;
> > > +	if (fcntl(fd, F_SETFD, flags) < 0)
> > > +		err_exit("setfd", errno);
> > > +	snprintf(fdstr, sizeof(fdstr), "%d", fd);
> > > +	execve(argv[0], newargv, NULL);
> > > +	return 0;
> > > +}
> > > diff --git a/tests/generic/484 b/tests/generic/484
> > > new file mode 100755
> > > index 00000000..76ebf1fd
> > > --- /dev/null
> > > +++ b/tests/generic/484
> > > @@ -0,0 +1,73 @@
> > > +#! /bin/bash
> > > +# FS QA Test 484
> > > +#
> > > +# This is testing
> > > +#
> > > +#  "Record locks are not inherited by a child created via fork(2),
> > > +#   but are preserved across an execve(2)."
> > > +#
> > > +# from fcntl(2) man page.
> > > +#
> > > +# Fixed by patch from Jeff Layton:
> > > +# locks: change POSIX lock ownership on execve when files_struct is displaced
> > > +#
> > > +# Author: "Daniel P. Berrangé" <berrange@redhat.com>
> > > +#
> > > +#-----------------------------------------------------------------------
> > > +# Copyright (c) 2018 Red Hat Inc.  All Rights Reserved.
> > > +#
> > > +# This program is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU General Public License as
> > > +# published by the Free Software Foundation.
> > > +#
> > > +# This program is distributed in the hope that it would be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program; if not, write the Free Software Foundation,
> > > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > > +#-----------------------------------------------------------------------
> > > +#
> > > +
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +tmp=/tmp/$$
> > > +status=1	# failure is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > +	cd /
> > > +	rm -f $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +
> > > +# remove previous $seqres.full before test
> > > +rm -f $seqres.full
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs generic
> > > +_supported_os Linux
> > > +_require_test
> > > +
> > > +# prepare a 4k testfile in TEST_DIR
> > > +$XFS_IO_PROG -f -c "pwrite -S 0xFF 0 4096" \
> > > +	$TEST_DIR/t_lock_execve_file >> $seqres.full 2>&1
> > > +
> > > +# run test programe
> > 
> > Unnecessary comment.
> > 
> > > +$here/src/t_locks_execve $TEST_DIR/t_lock_execve_file
> > 
> > I can fix the commit log and test description and the code style issue
> > on commit, no need to resend.
> > 
> > Thanks,
> > Eryu
> > 
> > > +
> > > +# success, all done
> > > +echo "Silence is golden"
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/484.out b/tests/generic/484.out
> > > new file mode 100644
> > > index 00000000..94f2f0bd
> > > --- /dev/null
> > > +++ b/tests/generic/484.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 484
> > > +Silence is golden
> > > diff --git a/tests/generic/group b/tests/generic/group
> > > index ea8e51b3..798321f9 100644
> > > --- a/tests/generic/group
> > > +++ b/tests/generic/group
> > > @@ -486,3 +486,4 @@
> > >  481 auto quick log metadata
> > >  482 auto metadata replay
> > >  483 auto quick log metadata
> > > +484 auto lock quick
> > > -- 
> > > 2.17.0.252.gfe0a9ea
> > > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jeff Layton <jlayton@poochiereds.net>