svn commit: r284029 - head/sys/kern

[John Baldwin]

On Friday, June 05, 2015 04:21:44 PM 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).
>   
>   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;

This should probably be volatile?

>  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);

It's not really safe to not use an atomic here but use one in runlock.
What happens if the compiler uses separate load - add - store here and
another thread does the atomic subtract after the load but before the
store?

> +}
> +
> +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);

Reading the variable after subtract is also racey.  You can use
atomic_fetchadd() instead (which is what refcount_release does).

-- 
John Baldwin