[PATCH 0/3] xfs_repair: pthread creation failure recovery fixes

[PATCH 0/3] xfs_repair: pthread creation failure recovery fixes

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

Hi all -

I recently had a bug report where xfs_repair in the dracut initramfs
environment was crashing.  It turned out to be due to systemd setting
up cgroup pids limits and pthread_create was failing.  The "real" fix
is to remove that limit, but it created a easily reproducable scenario
with which to test thread creation failure recovery.  A combination of
a small pids limit and MALLOC_PERTURB_ shook out a few bugs.  There's
more work to be done here since it should probably fall back to single
threads whenever we can't create a thread, but I only tackled the cases
that were already being handled gracefully instead of erroring out.

-Jeff

---

Jeff Mahoney (3):
  xfs_repair: clear pthread_t when pthread_create fails
  xfs_repair: add prefetch trace calls to debug thread creation failures
  xfs_repair: fix thread creation failure recovery

 repair/prefetch.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

-- 
2.7.1

[PATCH 1/3] xfs_repair: clear pthread_t when pthread_create fails

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

pf_queuing_worker and pf_create_prefetch_thread both try to handle
thread creation failure gracefully, but assume that pthread_create
doesn't modify the pthread_t when it fails.

>From the pthread_create man page:
On  success,  pthread_create() returns 0; on error, it returns an error
number, and the contents of *thread are undefined.

In fact, glibc's pthread_create writes the pthread_t value before
calling clone().  When we join the created threads in
cleanup_inode_prefetch and the cleanup stage of pf_queuing_worker, we
assume that if the pthread_t is nonzero that it's a valid thread handle
and end up crashing in pthread_join.

This patch zeros out the handle after pthread_create failure.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 repair/prefetch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/repair/prefetch.c b/repair/prefetch.c
index ff50606..044fab2 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -703,6 +703,7 @@ pf_queuing_worker(
 		if (err != 0) {
 			do_warn(_("failed to create prefetch thread: %s\n"),
 				strerror(err));
+			args->io_threads[i] = 0;
 			if (i == 0) {
 				pf_start_processing(args);
 				return NULL;
@@ -816,6 +817,7 @@ pf_create_prefetch_thread(
 	if (err != 0) {
 		do_warn(_("failed to create prefetch thread: %s\n"),
 			strerror(err));
+		args->queuing_thread = 0;
 		cleanup_inode_prefetch(args);
 	}
 
-- 
2.7.1

Re: [PATCH 1/3] xfs_repair: clear pthread_t when pthread_create fails

[Eric Sandeen]

On 1/16/17 10:39 PM, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> pf_queuing_worker and pf_create_prefetch_thread both try to handle
> thread creation failure gracefully, but assume that pthread_create
> doesn't modify the pthread_t when it fails.
> 
> From the pthread_create man page:
> On  success,  pthread_create() returns 0; on error, it returns an error
> number, and the contents of *thread are undefined.
> 
> In fact, glibc's pthread_create writes the pthread_t value before
> calling clone().  When we join the created threads in
> cleanup_inode_prefetch and the cleanup stage of pf_queuing_worker, we
> assume that if the pthread_t is nonzero that it's a valid thread handle
> and end up crashing in pthread_join.
> 
> This patch zeros out the handle after pthread_create failure.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

Sweeping up old patches, sorry for missing these.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  repair/prefetch.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index ff50606..044fab2 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -703,6 +703,7 @@ pf_queuing_worker(
>  		if (err != 0) {
>  			do_warn(_("failed to create prefetch thread: %s\n"),
>  				strerror(err));
> +			args->io_threads[i] = 0;
>  			if (i == 0) {
>  				pf_start_processing(args);
>  				return NULL;
> @@ -816,6 +817,7 @@ pf_create_prefetch_thread(
>  	if (err != 0) {
>  		do_warn(_("failed to create prefetch thread: %s\n"),
>  			strerror(err));
> +		args->queuing_thread = 0;
>  		cleanup_inode_prefetch(args);
>  	}
>  
> 

[PATCH 2/3] xfs_repair: add prefetch trace calls to debug thread creation failures

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

When debugging prefetch failures, it's useful to have thread creation
failure messages that are output as warnings on stderr in the trace
log as well.  It's also helpful to see when an AG gets queued behind
another one rather than having the thread started directly, which
has a separate trace line.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 repair/prefetch.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/repair/prefetch.c b/repair/prefetch.c
index 044fab2..37d60d6 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -703,6 +703,8 @@ pf_queuing_worker(
 		if (err != 0) {
 			do_warn(_("failed to create prefetch thread: %s\n"),
 				strerror(err));
+			pftrace("failed to create prefetch thread for AG %d: %s",
+				args->agno, strerror(err));
 			args->io_threads[i] = 0;
 			if (i == 0) {
 				pf_start_processing(args);
@@ -817,6 +819,8 @@ pf_create_prefetch_thread(
 	if (err != 0) {
 		do_warn(_("failed to create prefetch thread: %s\n"),
 			strerror(err));
+		pftrace("failed to create prefetch thread for AG %d: %s",
+			args->agno, strerror(err));
 		args->queuing_thread = 0;
 		cleanup_inode_prefetch(args);
 	}
@@ -882,8 +886,11 @@ start_inode_prefetch(
 		if (prev_args->prefetch_done) {
 			if (!pf_create_prefetch_thread(args))
 				args = NULL;
-		} else
+		} else {
 			prev_args->next_args = args;
+			pftrace("queued AG %d after AG %d",
+				args->agno, prev_args->agno);
+		}
 		pthread_mutex_unlock(&prev_args->lock);
 	}
 
-- 
2.7.1

Re: [PATCH 2/3] xfs_repair: add prefetch trace calls to debug thread creation failures

[Eric Sandeen]

On 1/16/17 10:39 PM, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> When debugging prefetch failures, it's useful to have thread creation
> failure messages that are output as warnings on stderr in the trace
> log as well.  It's also helpful to see when an AG gets queued behind
> another one rather than having the thread started directly, which
> has a separate trace line.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  repair/prefetch.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index 044fab2..37d60d6 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -703,6 +703,8 @@ pf_queuing_worker(
>  		if (err != 0) {
>  			do_warn(_("failed to create prefetch thread: %s\n"),
>  				strerror(err));
> +			pftrace("failed to create prefetch thread for AG %d: %s",
> +				args->agno, strerror(err));
>  			args->io_threads[i] = 0;
>  			if (i == 0) {
>  				pf_start_processing(args);
> @@ -817,6 +819,8 @@ pf_create_prefetch_thread(
>  	if (err != 0) {
>  		do_warn(_("failed to create prefetch thread: %s\n"),
>  			strerror(err));
> +		pftrace("failed to create prefetch thread for AG %d: %s",
> +			args->agno, strerror(err));
>  		args->queuing_thread = 0;
>  		cleanup_inode_prefetch(args);
>  	}
> @@ -882,8 +886,11 @@ start_inode_prefetch(
>  		if (prev_args->prefetch_done) {
>  			if (!pf_create_prefetch_thread(args))
>  				args = NULL;
> -		} else
> +		} else {
>  			prev_args->next_args = args;
> +			pftrace("queued AG %d after AG %d",
> +				args->agno, prev_args->agno);
> +		}
>  		pthread_mutex_unlock(&prev_args->lock);
>  	}
>  
> 

[PATCH 3/3] xfs_repair: fix thread creation failure recovery

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

When pf_create_prefetch_thread fails, it tears down the args struct
and frees it.  This causes a use-after-free in prefetch_ag_range, which
then passes the now-invalid pointer to start_inode_prefetch.  The struct
is only freed when the queuing thread can't be started.  When we can't
start even one worker thread, we mark the args ready for processing and
allow it to proceed single-threaded.  Unfortunately, this only marks
the current args ready for processing and since we return immediately,
the call to pf_create_prefetch_thread at the end of pf_queuing_worker
never gets called and we wait forever for prefetch to start on the
next AG.

This patch factors out the cleanup into a new pf_skip_prefetch_thread
that is called when we fail to create either the queuing thread or
the first of the workers.  It marks the args ready for processing, marks
it done so start_inode_prefetch doesn't add another AG to the list, and
tries to start a new thread for the next AG in the list.  We also clear
->next_args and check for it in cleanup_inode_prefetch so this condition
is easier to catch should it arise again.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 repair/prefetch.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/repair/prefetch.c b/repair/prefetch.c
index 37d60d6..4c74b6e 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -679,11 +679,32 @@ static int
 pf_create_prefetch_thread(
 	prefetch_args_t		*args);
 
+/*
+ * If we fail to create the queuing thread or can't create even one
+ * prefetch thread, we need to let processing continue without it.
+ */
+static void
+pf_skip_prefetch_thread(prefetch_args_t *args)
+{
+	prefetch_args_t *next;
+
+	pthread_mutex_lock(&args->lock);
+	args->prefetch_done = 1;
+	pf_start_processing(args);
+	next = args->next_args;
+	args->next_args = NULL;
+	pthread_mutex_unlock(&args->lock);
+
+	if (next)
+		pf_create_prefetch_thread(next);
+}
+
 static void *
 pf_queuing_worker(
 	void			*param)
 {
 	prefetch_args_t		*args = param;
+	prefetch_args_t		*next_args;
 	int			num_inos;
 	ino_tree_node_t		*irec;
 	ino_tree_node_t		*cur_irec;
@@ -707,7 +728,7 @@ pf_queuing_worker(
 				args->agno, strerror(err));
 			args->io_threads[i] = 0;
 			if (i == 0) {
-				pf_start_processing(args);
+				pf_skip_prefetch_thread(args);
 				return NULL;
 			}
 			/*
@@ -798,11 +819,13 @@ pf_queuing_worker(
 	ASSERT(btree_is_empty(args->io_queue));
 
 	args->prefetch_done = 1;
-	if (args->next_args)
-		pf_create_prefetch_thread(args->next_args);
-
+	next_args = args->next_args;
+	args->next_args = NULL;
 	pthread_mutex_unlock(&args->lock);
 
+	if (next_args)
+		pf_create_prefetch_thread(next_args);
+
 	return NULL;
 }
 
@@ -822,7 +845,7 @@ pf_create_prefetch_thread(
 		pftrace("failed to create prefetch thread for AG %d: %s",
 			args->agno, strerror(err));
 		args->queuing_thread = 0;
-		cleanup_inode_prefetch(args);
+		pf_skip_prefetch_thread(args);
 	}
 
 	return err == 0;
@@ -884,14 +907,15 @@ start_inode_prefetch(
 	} else {
 		pthread_mutex_lock(&prev_args->lock);
 		if (prev_args->prefetch_done) {
+			pthread_mutex_unlock(&prev_args->lock);
 			if (!pf_create_prefetch_thread(args))
 				args = NULL;
 		} else {
 			prev_args->next_args = args;
 			pftrace("queued AG %d after AG %d",
 				args->agno, prev_args->agno);
+			pthread_mutex_unlock(&prev_args->lock);
 		}
-		pthread_mutex_unlock(&prev_args->lock);
 	}
 
 	return args;
@@ -1066,6 +1090,8 @@ cleanup_inode_prefetch(
 
 	pftrace("AG %d prefetch done", args->agno);
 
+	ASSERT(args->next_args == NULL);
+
 	pthread_mutex_destroy(&args->lock);
 	pthread_cond_destroy(&args->start_reading);
 	pthread_cond_destroy(&args->start_processing);
-- 
2.7.1