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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > +#
> > +#-----------------------------------------------------------------------
> > +# 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
