lkml.org 
[lkml]   [2005]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectSubject: [RFC][PATCH] Fix for unsafe notifier chain mechanism
    From
    Date
    Hello,

    In 2.6.14, the notifier chains are unsafe. notifier_call_chain() walks through
    the list of a call chain without any protection.

    Alan Stern <stern@rowland.harvard.edu> brought the issue and suggested a fix
    in lkml on Oct 24 2005:
    http://marc.theaimsgroup.com/?l=linux-kernel&m=113018709002036&w=2

    There was a lot of discussion on that thread regarding the issue, and
    following were the conclusions about the requirements of the notifier
    call mechanism:

    - The chain list has to be protected in all the places where the
    list is accessed.
    - we need a new notifier_head data structure to encompass the head
    of the notifier chain and a semaphore that protects the list.
    - There should be two types of call chains: one that is called in
    a process context and another that is called in atomic/interrupt
    context.
    - No lock should be acquired in notifier_call_chain() for an
    atomic-type chain.
    - notifier_chain_register() and notifier_chain_unregister() should
    be called only from process context.
    - notifier_chain_unregister() should _not_ be called from a
    callout routine.

    This patch is a modified version of Alan's original patch and meets these
    requirements. The patch is not complete, since all the notifier call
    chain definitions have to changed to use the new notifier_head data
    structure.

    Alan and I did think about changing the data structure to use list_head, but
    deferred it (as a cleanup) as it is not directly tied with what Alan was
    trying to fix.
    ----

    Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
    Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
    -----

    include/linux/notifier.h | 69 +++++++++++++++++++++++++++++---
    kernel/sys.c | 101 ++++++++++++++++++++++++++++-------------------
    2 files changed, 126 insertions(+), 44 deletions(-)

    Index: l2614-notifiers/include/linux/notifier.h
    ===================================================================
    --- l2614-notifiers.orig/include/linux/notifier.h
    +++ l2614-notifiers/include/linux/notifier.h
    @@ -10,25 +10,84 @@
    #ifndef _LINUX_NOTIFIER_H
    #define _LINUX_NOTIFIER_H
    #include <linux/errno.h>
    +#include <linux/rwsem.h>
    +
    +/*
    + * Notifier chains are of two types:
    + * Atomic notifier chains: Chain callbacks run in interrupt/atomic
    + * context. Callouts are not allowed to block.
    + * Blocking notifier chains: Chain callback run in process context.
    + * Callouts are allowed to block.
    + *
    + * Type of a chain is defined in its head.
    + *
    + * notifier_chain_register() and notifier_chain_unregister() should be
    + * called only from process context.
    + *
    + * notifier_chain_unregister() _should not_ be called from the
    + * corresponding call chain.
    + *
    + */
    +enum notifier_type {
    + ATOMIC_NOTIFIER,
    + BLOCKING_NOTIFIER,
    +};

    struct notifier_block
    {
    - int (*notifier_call)(struct notifier_block *self, unsigned long, void *);
    + int (*notifier_call)(struct notifier_block *, unsigned long, void *);
    struct notifier_block *next;
    int priority;
    };

    +struct notifier_head {
    + enum notifier_type type;
    + struct rw_semaphore rwsem;
    + struct notifier_block *head;
    +};
    +
    +#define NOTIFIER_HEAD_INIT(name, head_type) { \
    + .type = head_type, \
    + .rwsem = __RWSEM_INITIALIZER((name).rwsem), \
    + .head = NULL }
    +
    +#define NOTIFIER_HEAD(name, head_type) \
    + struct notifier_head name = NOTIFIER_HEAD_INIT(name, head_type)
    +
    +#define INIT_NOTIFIER_HEAD(name, head_type) do { \
    + (ptr)->type = head_type; \
    + init_rwsem(&(ptr)->rwsem); \
    + ptr->head = NULL; \
    +} while (0)
    +
    +#define ATOMIC_NOTIFIER_HEAD_INIT(name) \
    + NOTIFIER_HEAD_INIT(name, ATOMIC_NOTIFIER)
    +#define ATOMIC_NOTIFIER_HEAD(name) \
    + NOTIFIER_HEAD(name, ATOMIC_NOTIFIER)
    +#define ATOMIC_INIT_NOTIFIER_HEAD(name) \
    + INIT_NOTIFIER_HEAD(name, ATOMIC_NOTIFIER)
    +
    +#define BLOCKING_NOTIFIER_HEAD_INIT(name) \
    + NOTIFIER_HEAD_INIT(name, BLOCKING_NOTIFIER)
    +#define BLOCKING_NOTIFIER_HEAD(name) \
    + NOTIFIER_HEAD(name, BLOCKING_NOTIFIER)
    +#define BLOCKING_INIT_NOTIFIER_HEAD(name) \
    + INIT_NOTIFIER_HEAD(name, BLOCKING_NOTIFIER)

    #ifdef __KERNEL__

    -extern int notifier_chain_register(struct notifier_block **list, struct notifier_block *n);
    -extern int notifier_chain_unregister(struct notifier_block **nl, struct notifier_block *n);
    -extern int notifier_call_chain(struct notifier_block **n, unsigned long val, void *v);
    +extern int notifier_chain_register(struct notifier_head *,
    + struct notifier_block *);
    +extern int notifier_chain_unregister(struct notifier_head *,
    + struct notifier_block *);
    +extern int notifier_call_chain(struct notifier_head *,
    + unsigned long val, void *v);

    #define NOTIFY_DONE 0x0000 /* Don't care */
    #define NOTIFY_OK 0x0001 /* Suits me */
    #define NOTIFY_STOP_MASK 0x8000 /* Don't call further */
    -#define NOTIFY_BAD (NOTIFY_STOP_MASK|0x0002) /* Bad/Veto action */
    +#define NOTIFY_BAD (NOTIFY_STOP_MASK|0x0002)
    + /* Bad/Veto action */
    /*
    * Clean way to return from the notifier and stop further calls.
    */
    Index: l2614-notifiers/kernel/sys.c
    ===================================================================
    --- l2614-notifiers.orig/kernel/sys.c
    +++ l2614-notifiers/kernel/sys.c
    @@ -92,31 +92,35 @@ int cad_pid = 1;
    * and the like.
    */

    -static struct notifier_block *reboot_notifier_list;
    -static DEFINE_RWLOCK(notifier_lock);
    +static BLOCKING_NOTIFIER_HEAD(reboot_notifier_list);

    /**
    * notifier_chain_register - Add notifier to a notifier chain
    - * @list: Pointer to root list pointer
    + * @nh: Pointer to head of the notifier chain
    * @n: New entry in notifier chain
    *
    * Adds a notifier to a notifier chain.
    + * Must be called from process context.
    *
    * Currently always returns zero.
    */

    -int notifier_chain_register(struct notifier_block **list, struct notifier_block *n)
    +int notifier_chain_register(struct notifier_head *nh, struct notifier_block *n)
    {
    - write_lock(&notifier_lock);
    - while(*list)
    - {
    - if(n->priority > (*list)->priority)
    - break;
    - list= &((*list)->next);
    - }
    - n->next = *list;
    - *list=n;
    - write_unlock(&notifier_lock);
    + struct notifier_block **nl;
    +
    + down_write(&nh->rwsem);
    + nl = &nh->head;
    + while ((*nl) != NULL) {
    + if (n->priority > (*nl)->priority)
    + break;
    + nl = &((*nl)->next);
    + }
    + rcu_assign_pointer(n->next, *nl);
    + rcu_assign_pointer(*nl, n);
    + up_write(&nh->rwsem);
    + if (nh->type == ATOMIC_NOTIFIER)
    + synchronize_rcu();
    return 0;
    }

    @@ -124,28 +128,32 @@ EXPORT_SYMBOL(notifier_chain_register);

    /**
    * notifier_chain_unregister - Remove notifier from a notifier chain
    - * @nl: Pointer to root list pointer
    + * @nh: Pointer to head of the notifier chain
    * @n: New entry in notifier chain
    *
    * Removes a notifier from a notifier chain.
    + * Must be called from process context.
    *
    * Returns zero on success, or %-ENOENT on failure.
    */
    -
    -int notifier_chain_unregister(struct notifier_block **nl, struct notifier_block *n)
    +int notifier_chain_unregister(struct notifier_head *nh,
    + struct notifier_block *n)
    {
    - write_lock(&notifier_lock);
    - while((*nl)!=NULL)
    - {
    - if((*nl)==n)
    - {
    - *nl=n->next;
    - write_unlock(&notifier_lock);
    + struct notifier_block **nl;
    +
    + down_write(&nh->rwsem);
    + nl = &nh->head;
    + while ((*nl) != NULL) {
    + if ((*nl) == n) {
    + rcu_assign_pointer(*nl, n->next);
    + up_write(&nh->rwsem);
    + if (nh->type == ATOMIC_NOTIFIER)
    + synchronize_rcu();
    return 0;
    }
    - nl=&((*nl)->next);
    + nl = &((*nl)->next);
    }
    - write_unlock(&notifier_lock);
    + up_write(&nh->rwsem);
    return -ENOENT;
    }

    @@ -153,12 +161,18 @@ EXPORT_SYMBOL(notifier_chain_unregister)

    /**
    * notifier_call_chain - Call functions in a notifier chain
    - * @n: Pointer to root pointer of notifier chain
    + * @nh: Pointer to the head of notifier chain
    * @val: Value passed unmodified to notifier function
    * @v: Pointer passed unmodified to notifier function
    *
    * Calls each function in a notifier chain in turn.
    *
    + * If @nh points to an %ATOMIC_NOTIFIER_HEAD then this routine may
    + * be called in any context, as it will not sleep.
    + *
    + * If @nh points to a %BLOCKING_NOTIFIER_HEAD then this routine may
    + * be called only in process context.
    + *
    * If the return value of the notifier can be and'd
    * with %NOTIFY_STOP_MASK, then notifier_call_chain
    * will return immediately, with the return value of
    @@ -167,20 +181,29 @@ EXPORT_SYMBOL(notifier_chain_unregister)
    * of the last notifier function called.
    */

    -int notifier_call_chain(struct notifier_block **n, unsigned long val, void *v)
    +int notifier_call_chain(struct notifier_head *nh, unsigned long val, void *v)
    {
    - int ret=NOTIFY_DONE;
    - struct notifier_block *nb = *n;
    + int ret = NOTIFY_DONE;
    + struct notifier_block *nb;

    - while(nb)
    - {
    - ret=nb->notifier_call(nb,val,v);
    - if(ret&NOTIFY_STOP_MASK)
    - {
    - return ret;
    - }
    - nb=nb->next;
    - }
    + if (!nh->head)
    + return ret;
    + if (nh->type == ATOMIC_NOTIFIER)
    + rcu_read_lock();
    + else
    + down_read(&nh->rwsem);
    + nb = rcu_dereference(nh->head);
    + while (nb) {
    + ret = nb->notifier_call(nb, val, v);
    + if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
    + goto done;
    + nb = rcu_dereference(nb->next);
    + }
    +done:
    + if (nh->type == ATOMIC_NOTIFIER)
    + rcu_read_unlock();
    + else
    + up_read(&nh->rwsem);
    return ret;
    }

    --

    ----------------------------------------------------------------------
    Chandra Seetharaman | Be careful what you choose....
    - sekharan@us.ibm.com | .......you may get it.
    ----------------------------------------------------------------------

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-11-12 00:46    [W:0.040 / U:33.788 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site