Re: run-elf_cntl_gelf_getshdr.sh: Resource deadlock avoided.

[Mark Wielaard <mjw at redhat dot com>]

On Sun, 2013-11-03 at 15:40 +0100, Kurt Roeckx wrote:
> When running the test suite with --enable-thread-safety I get the
> following error in the FDREAD case:
> test-elf_cntl_gelf_getshdr: elf_readall.c:73: __libelf_readall: Unexpected error: Resource deadlock avoided.
> Aborted

Hmmm. We keep getting people trying out that configure option. But since
nobody is really testing or using it we really should discourage people
from using it. I added the following warning:

configure: WARNING: thread-safety is EXPERIMENTAL tests might fail.

People might still ignore it, but at least we did warn them.

> (gdb) frame 4
> #4  0x00007ffff7bcb072 in __libelf_readall (elf=elf(a)entry=0x603010) at elf_readall.c:73
> 73        rwlock_wrlock (elf->lock);
> 
> elf_cntl() also already did a rwlock_wrlock (elf->lock); at elf_cntl.c:55
> 
> The only other caller of __libelf_readall() seems to be elf_rawfile()
> which takes a lock just after the __libelf_readall() call.

elf_rawfile takes a rwlock_rdlock, __libelf_readall uses a
rwlock_wrlock.

As far as I understand rdlocks may nest, but wrlock that nest might
cause deadlocks (even when "upgrading" an rdlock to a wrlock).

I think the correct solution is to change to scope of the rwlock_wrlock
in elf_cntl to just the ELF_C_FDDONE case. But I might be missing which
field are protected by the lock. Does the following look sane and work
for you?

diff --git a/libelf/elf_cntl.c b/libelf/elf_cntl.c
index a3c5805..3b29571 100644
--- a/libelf/elf_cntl.c
+++ b/libelf/elf_cntl.c
@@ -52,8 +52,6 @@ elf_cntl (elf, cmd)
       return -1;
     }
 
-  rwlock_wrlock (elf->lock);
-
   switch (cmd)
     {
     case ELF_C_FDREAD:
@@ -68,7 +66,9 @@ elf_cntl (elf, cmd)
 
     case ELF_C_FDDONE:
       /* Mark the file descriptor as not usable.  */
+      rwlock_wrlock (elf->lock);
       elf->fildes = -1;
+      rwlock_unlock (elf->lock);
       break;
 
     default:
@@ -77,7 +77,5 @@ elf_cntl (elf, cmd)
       break;
     }
 
-  rwlock_unlock (elf->lock);
-
   return result;
 }

>From 09c9bb9253dc923ea772e2232f896dbc8ab67e83 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 6 Nov 2013 12:21:32 +0100
Subject: [PATCH] configure.ac: Add warning that --enable-thread-safety is experimental.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 ChangeLog    |    5 +++++
 configure.ac |    7 +++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 026ce1e..f808413 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2013-11-06  Mark Wielaard  <mjw@redhat.com>
+
+	* configure.ac (--enable-thread-safety): Add AC_MSG_WARN experimental
+	option.
+
 2013-11-01  Michael Forney  <mforney@mforney.org>
 
 	* configure.ac: Call AM_PROG_AR and AC_CHECK_TOOL for readelf and nm.
diff --git a/configure.ac b/configure.ac
index ee2ce26..4023f31 100644
--- a/configure.ac
+++ b/configure.ac
@@ -60,10 +60,13 @@ AC_DEFINE_UNQUOTED(DEFAULT_AR_DETERMINISTIC, $default_ar_deterministic,
 		   [Should ar and ranlib use -D behavior by default?])
 
 AC_ARG_ENABLE([thread-safety],
-AS_HELP_STRING([--enable-thread-safety], [enable thread safety of libraries]),
-use_locks=$enableval, use_locks=no)
+AS_HELP_STRING([--enable-thread-safety],
+               [enable thread safety of libraries EXPERIMENTAL]),
+               use_locks=$enableval, use_locks=no)
 AM_CONDITIONAL(USE_LOCKS, test "$use_locks" = yes)
 AS_IF([test "$use_locks" = yes], [AC_DEFINE(USE_LOCKS)])
+AS_IF([test "$use_locks" = yes],
+      [AC_MSG_WARN([thread-safety is EXPERIMENTAL tests might fail.])])
 
 AH_TEMPLATE([USE_LOCKS], [Defined if libraries should be thread-safe.])
 
-- 
1.7.1