Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 5 Jun 2015 18:54:18 +0200
From:      Mateusz Guzik <mjguzik@gmail.com>
To:        Sean Bruno <sbruno@FreeBSD.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r284029 - head/sys/kern
Message-ID:  <20150605165417.GA13205@dft-labs.eu>
In-Reply-To: <201506051621.t55GLiKp095005@svn.freebsd.org>
References:  <201506051621.t55GLiKp095005@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jun 05, 2015 at 04:21:44PM +0000, Sean Bruno wrote:
> Author: sbruno
> Date: Fri Jun  5 16:21:43 2015
> New Revision: 284029
> URL: https://svnweb.freebsd.org/changeset/base/284029
> 
> Log:
>   This change uses a reader count instead of holding the mutex for the
>   interpreter list to avoid the problem of holding a non-sleep lock during
>   a page fault as reported by witness.  In addition, it consistently uses
>   memset()/memcpy() instead of bzero()/bcopy() except in the case where
>   bcopy() is required (i.e. overlapping copy).

What lock was that? If it has to be held, why not use an sx lock?

The change looks quite buggy.

>   
>   Differential Revision:	https://reviews.freebsd.org/D2123
>   Submitted by:	sson
>   MFC after:	2 weeks
>   Relnotes:	Yes
> 
> Modified:
>   head/sys/kern/imgact_binmisc.c
> 
> Modified: head/sys/kern/imgact_binmisc.c
> ==============================================================================
> --- head/sys/kern/imgact_binmisc.c	Fri Jun  5 16:02:24 2015	(r284028)
> +++ head/sys/kern/imgact_binmisc.c	Fri Jun  5 16:21:43 2015	(r284029)
> @@ -1,5 +1,5 @@
>  /*-
> - * Copyright (c) 2013, Stacey D. Son
> + * Copyright (c) 2013-15, Stacey D. Son
>   * All rights reserved.
>   *
>   * Redistribution and use in source and binary forms, with or without
> @@ -41,6 +41,9 @@ __FBSDID("$FreeBSD$");
>  #include <sys/malloc.h>
>  #include <sys/mutex.h>
>  #include <sys/sysctl.h>
> +#include <sys/sx.h>
> +
> +#include <machine/atomic.h>
>  
>  /**
>   * Miscellaneous binary interpreter image activator.
> @@ -95,11 +98,68 @@ static SLIST_HEAD(, imgact_binmisc_entry
>  	SLIST_HEAD_INITIALIZER(interpreter_list);
>  
>  static int interp_list_entry_count = 0;
> -
> +static int interp_list_rcount = 0;
>  static struct mtx interp_list_mtx;
>  
>  int imgact_binmisc_exec(struct image_params *imgp);
>  
> +static inline void
> +interp_list_rlock()
> +{
> +
> +    /* Don't use atomic_add() here.  We must block if the mutex is held. */
> +    mtx_lock(&interp_list_mtx);
> +    interp_list_rcount++;
> +    mtx_unlock(&interp_list_mtx);
> +}
> +
> +static inline void
> +interp_list_runlock()
> +{
> +
> +    /* Decrement the reader count and wake up one writer, if any. */
> +    atomic_subtract_int(&interp_list_rcount, 1);
> +    if (interp_list_rcount == 0)
> +        wakeup_one(&interp_list_rcount);
> +}
> +

interp_list_rcount manipulation between these 2 is not safe. This
effectively is:

A				B
interp_list_rcount++		atomic_subtract_int(&interp_list_rcount,
1);

As such updates can be lost. There is also potential for bonus wakeup.

> +static inline void
> +interp_list_wlock()
> +{
> +
> +    /* Wait until there are no readers. */
> +    mtx_lock(&interp_list_mtx);
> +    while (interp_list_rcount)
> +        mtx_sleep(&interp_list_rcount, &interp_list_mtx, 0, "IABM", 0);
> +}
> +

Given that runlock does not take the lock, this func can sleep after
wakeup_one() thus blocking the thread with nobody to wake it up.

-- 
Mateusz Guzik <mjguzik gmail.com>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150605165417.GA13205>