diff mbox series

[01/10,v3] libelf: Fix deadlock in __libelf_readall

Message ID 20240802233847.690564-1-amerey@redhat.com
State Committed
Delegated to: Mark Wielaard
Headers
Series [01/10,v3] libelf: Fix deadlock in __libelf_readall |

Commit Message

Aaron Merey Aug. 2, 2024, 11:38 p.m. UTC 
  From: Heather McIntyre <hsm2@rice.edu>

Apply locking during __libelf_readall.

Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
Signed-off-by: Aaron Merey <amerey@redhat.com>
Signed-off-by: Mark Wielaard <mark@klomp.org>

---
v3 changes:

Update comments and change order or child lock aquisition.

 libelf/common.h      | 24 ++++++++++++++----------
 libelf/elf_readall.c |  4 ++--
 2 files changed, 16 insertions(+), 12 deletions(-)

Comments

Mark Wielaard Aug. 15, 2024, 4:12 p.m. UTC | #1 
Hi Aaron,

On Fri, 2024-08-02 at 19:38 -0400, Aaron Merey wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
> Apply locking during __libelf_readall.
> 
> Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> 
> ---
> v3 changes:
> 
> Update comments and change order or child lock aquisition.

This looks OK, it addresses all my comments from v2.

Thanks,

Mark
Aaron Merey Aug. 21, 2024, 12:25 a.m. UTC | #2 
Hi Mark,

On Thu, Aug 15, 2024 at 12:13 PM Mark Wielaard <mark@klomp.org> wrote:
>
> On Fri, 2024-08-02 at 19:38 -0400, Aaron Merey wrote:
> > From: Heather McIntyre <hsm2@rice.edu>
> >
> > Apply locking during __libelf_readall.
> >
> > Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
> > Signed-off-by: Aaron Merey <amerey@redhat.com>
> > Signed-off-by: Mark Wielaard <mark@klomp.org>
> >
> > ---
> > v3 changes:
> >
> > Update comments and change order or child lock aquisition.
>
> This looks OK, it addresses all my comments from v2.

Thanks, pushed as commit f3de289b508.

Aaron
diff mbox series

Patch

diff --git a/libelf/common.h b/libelf/common.h
index 9b2a856d..8e33a70e 100644
--- a/libelf/common.h
+++ b/libelf/common.h
@@ -89,30 +89,34 @@  allocate_elf (int fildes, void *map_address, int64_t offset, size_t maxsize,
 }
 
 
-/* Acquire lock for the descriptor and all children.  */
+/* Caller must hold a lock for ELF. If there are children then a lock
+   will be acquired for each of them (recursively).  */
 static void
 __attribute__ ((unused))
-libelf_acquire_all (Elf *elf)
+libelf_acquire_all_children (Elf *elf)
 {
-  rwlock_wrlock (elf->lock);
-
   if (elf->kind == ELF_K_AR)
     {
       Elf *child = elf->state.ar.children;
 
       while (child != NULL)
 	{
+	  rwlock_wrlock (child->lock);
+
 	  if (child->ref_count != 0)
-	    libelf_acquire_all (child);
+	    libelf_acquire_all_children (child);
+
 	  child = child->next;
 	}
     }
 }
 
-/* Release own lock and those of the children.  */
+
+/* Caller must hold a lock for ELF. If there are children then a lock
+   will be released for each of them (recursively).  */
 static void
 __attribute__ ((unused))
-libelf_release_all (Elf *elf)
+libelf_release_all_children (Elf *elf)
 {
   if (elf->kind == ELF_K_AR)
     {
@@ -121,12 +125,12 @@  libelf_release_all (Elf *elf)
       while (child != NULL)
 	{
 	  if (child->ref_count != 0)
-	    libelf_release_all (child);
+	    libelf_release_all_children (child);
+
+	  rwlock_unlock (child->lock);
 	  child = child->next;
 	}
     }
-
-  rwlock_unlock (elf->lock);
 }
 
 
diff --git a/libelf/elf_readall.c b/libelf/elf_readall.c
index d0f9a28c..4ef8fe97 100644
--- a/libelf/elf_readall.c
+++ b/libelf/elf_readall.c
@@ -84,7 +84,7 @@  __libelf_readall (Elf *elf)
 
       /* If this is an archive and we have derived descriptors get the
 	 locks for all of them.  */
-      libelf_acquire_all (elf);
+      libelf_acquire_all_children (elf);
 
       if (elf->maximum_size == ~((size_t) 0))
 	{
@@ -141,7 +141,7 @@  __libelf_readall (Elf *elf)
 	__libelf_seterrno (ELF_E_NOMEM);
 
       /* Free the locks on the children.  */
-      libelf_release_all (elf);
+      libelf_release_all_children (elf);
     }
 
   rwlock_unlock (elf->lock);