* Re: 2.6.35-rc2-git1 - include/linux/cgroup.h:534 invoked rcu_dereference_check() without protection!
From: Paul E. McKenney @ 2010-06-08 0:19 UTC (permalink / raw)
To: Miles Lane
Cc: Vivek Goyal, Eric Paris, Lai Jiangshan, Ingo Molnar,
Peter Zijlstra, LKML, nauman, eric.dumazet, netdev, Jens Axboe,
Gui Jianfeng, Li Zefan, Johannes Berg
In-Reply-To: <AANLkTin2pPqOUx--9fIX3BH3e-cU6oCRufijcx_4ozx5@mail.gmail.com>
On Mon, Jun 07, 2010 at 02:14:25PM -0400, Miles Lane wrote:
> Hi All,
>
> I just reproduced a warning I reported quite a while ago. Is a patch
> for this in the pipeline?
I proposed a patch, thinking that it was a false positive. Peter Zijlstra
pointed out that there was a real race, and proposed an alternative patch,
which may be found at http://lkml.org/lkml/2010/4/22/603.
Could you please test Peter's patch and let us know if it cures the problem?
Thanx, Paul
> [ 0.167267] [ INFO: suspicious rcu_dereference_check() usage. ]
> [ 0.167396] ---------------------------------------------------
> [ 0.167526] include/linux/cgroup.h:534 invoked
> rcu_dereference_check() without protection!
> [ 0.167728]
> [ 0.167729] other info that might help us debug this:
> [ 0.167731]
> [ 0.168092]
> [ 0.168093] rcu_scheduler_active = 1, debug_locks = 1
> [ 0.168337] no locks held by watchdog/0/5.
> [ 0.168462]
> [ 0.168463] stack backtrace:
> [ 0.168704] Pid: 5, comm: watchdog/0 Not tainted 2.6.35-rc2-git1 #8
> [ 0.168834] Call Trace:
> [ 0.168965] [<ffffffff81064e9c>] lockdep_rcu_dereference+0x9d/0xa5
> [ 0.169100] [<ffffffff8102c1ce>] task_subsys_state+0x59/0x70
> [ 0.169232] [<ffffffff8103189b>] __sched_setscheduler+0x19d/0x2f8
> [ 0.169365] [<ffffffff8102a5ef>] ? need_resched+0x1e/0x28
> [ 0.169497] [<ffffffff813c7d01>] ? schedule+0x586/0x619
> [ 0.169628] [<ffffffff81081c33>] ? watchdog+0x0/0x8c
> [ 0.169758] [<ffffffff81031a11>] sched_setscheduler+0xe/0x10
> [ 0.169889] [<ffffffff81081c5d>] watchdog+0x2a/0x8c
> [ 0.170010] [<ffffffff81081c33>] ? watchdog+0x0/0x8c
> [ 0.170141] [<ffffffff81054a82>] kthread+0x89/0x91
> [ 0.170274] [<ffffffff81003054>] kernel_thread_helper+0x4/0x10
> [ 0.170405] [<ffffffff813ca480>] ? restore_args+0x0/0x30
> [ 0.170536] [<ffffffff810549f9>] ? kthread+0x0/0x91
> [ 0.170667] [<ffffffff81003050>] ? kernel_thread_helper+0x0/0x10
> [ 0.176751] lockdep: fixing up alternatives.
^ permalink raw reply
* [PATCH][RFC] Introduction to sk_buff state checking
From: David VomLehn @ 2010-06-08 0:30 UTC (permalink / raw)
To: to, "netdev, netdev
Add state checking for sk_buffs
Several common errors in sk_buff usage can be caught by maintaining and
checking the sk_buff state. States are: free, allocated, and queued. Simply
reporting an error is not sufficient; it may be the previous state change
that was erroneous. Thus, the ability to record the location of the previous
state change is essential.
This patch was previously submitted and David Miller objected to to the
overhead of recording the previous state change. Specifically, he wrote:
The specifics are that if you showed me something that
added a "u16", at most, I'd go to bat for you and support
your work going upstream.
With only three states, only two bits are required to record the current
state but recording the location of the call that changed the state in
no more than 14 bits posed an interesting challenge. This patchset contains
the infrastructure required to map between a small value, the "callsite ID",
and the location of the call.
There are four patches in this patchset:
1. This introduction
2. A patch allowing the site of a call to be passed over multiple calls
without modifying the intervening functions.
3. A patch providing infrastructure that allows the location of a call,
the "callsite ID" to be encoded in a small integer.
4. Modifications to the sk_buff functions to add the two-bit state
and a 14-bit callsite ID, check the state, and report failures.
Restrictions
o Call site IDs are never reused, so it is possible to exceed the
maximum number of IDs by having a large number of call locations.
In addition, it does not recognize that the same module has been
unloaded and reloaded, so calls from the reloaded module will be
assigned new IDs. Detection of incorrect operations on an sk_buff
is not affected by exhaustion of call site IDs, but it may not be
possible to determine the location of the last operation.
CONFIG_DEBUG_SKB_ID_SIZE is set to reduce the sk_buff growth to 16
bits and should handle most cases. It could be made larger to allow
more call site IDs, if necessary.
o The callsite structures for a module will be freed when that module
is unloaded, even though sk_buffs may be using IDs corresponding to
those call sites. To allow useful error reporting, the call site
information in a module being unloaded is copied. If a module is
unloaded and CONFIG_CALLSITE_TERSE is enabled, the address of
the call site is not longer valid, so only the function name and
offset are printed. If CONFIG_CALLSITE_TERSE is enabled and the
is loaded, the address is also reported.
Signed-off-by: David VomLehn <dvomlehn@cisco.com>
^ permalink raw reply
* [PATCH][RFC] Infrastructure for out-of-band parameter passing
From: David VomLehn @ 2010-06-08 0:30 UTC (permalink / raw)
To: to, "netdev, netdev
Infrastructure to support out of band/indirect passing of data to functions.
It is useful at times to be able to pass data from one function to another
nested many stack frames more deeply than the passing function. Doing so
allows the interfaces in the intervening functions to be simpler, though
this "hidden" information passing risks increased complexity. In cases
where this is justified, this simple infrastructure provides the
functionality.
Out of band data passing is implemented by adding, for each instance,
an element to the task_struct that serves as the pointer to the top
of a OOB parameter stack. Data is made available by being pushed
on the OOB parameter stack by a function, and accessed via the top
element of the OOB parameter stack.
Signed-off-by: David VomLehn (dvomlehn@cisco.com)
---
include/linux/oobparam.h | 89 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 89 insertions(+), 0 deletions(-)
diff --git a/include/linux/oobparam.h b/include/linux/oobparam.h
new file mode 100644
index 0000000..6eaa04c
--- /dev/null
+++ b/include/linux/oobparam.h
@@ -0,0 +1,89 @@
+/*
+ * Definitions used to pass information across multiple stack frames
+ * within a single task.
+ *
+ * Copyright (C) 2010 Cisco Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * Author: David VomLehn
+ *
+ * These definitions allow one function to establish a context for another
+ * function without adding parameters to the intervening functions. It can,
+ * for example, supply state information from one function to be used in the
+ * case that some function nested several levels more deeply wants to report
+ * the context from the original function. Since it works in a way invisible
+ * to the intervening functions, passing information this way is less
+ * obvious than simply supplying parameters and it should be used sparingly.
+ *
+ * To use this:
+ * 1. Define a structure containing a struct oobparam:
+ * struct ctx {
+ * const char *name;
+ * OOBPARAM_FRAME(frame);
+ * ...
+ * };
+ * 2. Create an element in the task_struct to serve as the top of the
+ * oobparam stack:
+ * OOBPARAM_TOP(my_top);
+ * 3. Populate the structure in the function that has context to be passed.
+ * struct ctx ctx;
+ * ctx.name = __func__;
+ * 4. Surround the call to the next function with oobparam_push() and
+ * oobparam_pop():
+ * oobparam_push(&ctx.frame);
+ * myfunc();
+ * oobparam_pop(&ctx.frame);
+ * 5. In function that wants to use the parameter value, use OOBPARAM_CUR
+ * to retrieve the value:
+ * struct ctx *ctx;
+ * ctx = OOBPARAM_CUR(&my_top, struct ctx, frame);
+ * pr_info("Indirectly called by %s\n", ctx->name);
+ */
+
+#ifndef _LINUX_OOBPARAM_H_
+#define _LINUX_OOBPARAM_H_
+
+struct oobparam {
+ struct oobparam *next;
+};
+
+#define OOBPARAM_TOP(name) struct oobparam name;
+#define OOBPARAM_FRAME(name) struct oobparam name;
+
+/**
+ * oobparam_push - push an out of band parameter frame on the OOB param stack
+ * @head Pointer to the OOB parameter stack top, which must be in the
+ * task structure.
+ * @frame Pointer to the OOB parameter frame, generally embedded in
+ * another structure
+ */
+static inline void oobparam_push(struct oobparam *top, struct oobparam *frame)
+{
+ frame->next = top;
+ /* We need to ensure that the pointer in the frame is set prior to
+ * the pointer to the top in case we handle an interrupt in between
+ * the two stores. */
+ wmb();
+ top->next = frame;
+}
+
+static inline void oobparam_pop(struct oobparam *top)
+{
+ top->next = top->next->next;
+}
+
+#define OOBPARAM_CUR(top, type, frame) container_of((top)->next, type, frame)
+#endif
^ permalink raw reply related
* [PATCH][RFC] Infrastructure for compact call location representation
From: David VomLehn @ 2010-06-08 0:30 UTC (permalink / raw)
To: to, "netdev, netdev
This patch allows the location of a call to be recorded as a small integer,
with each call location ("callsite") assigned a new value the first time
the code in that location is executed. Locations can be recorded as a
an address or as a __FILE__/__LINE__ pair. The later is easier to read, but
requires significantly more space.
The goal here was to record the location in very few bits but, at the same
time, to have minimal overhead. The key observation towards achieving this
goals is to note that there are are far fewer locations where calls of
interest are made than there are addresses. If the site of each call of
interest is assigned a unique ID, and there are fewer than n of them,
only log2(n) bits are required to identify the call site. If the IDs
are assigned dynamically and most call sites aren't reached, you can get
by with even fewer bits.
This is debugging code and callsite IDs are generally only used when failures
are detected, so though the mapping from a callsite location to a callsite ID
must be fast, speed is not as critical for the reverse mapping. Also, an ID
is assigned to a callsite just once, so it is acceptable to take a while to
assign an ID, but things should run with minimal delay if an ID is already
assigned.
The major implementation pieces are:
1. Two macros are provided for use in wrapping functions that are to
be instrumented. CALLSITE_CALL is for functions that return values,
CALLSITE_CALL_VOID is used for functions that do not.
2. The call site infrastructure consists of three data structures:
a. A statically allocated struct callsite_id holds the ID for
the call site.
b. A statically allocated struct callsite_static holds
information which is constant for each callsite. The call site
ID could be combined with this, but by separating them I hope
to avoid polluting the cache with this very cold information.
c. A struct callsite_frame builds on the oobparam infrastructure
and holds the call site ID. This is assigned at this time
if this had not previously been done. This will be pushed on
the OOB parameter stack before calling the skb_* function
and popped after it returns.
3. A callsite_top structure is added to task_struct. When a call site
is entered, its callsite_frame is pushed on the call site stack.
4. When a function needs to know the call site ID so it can be stored,
it gets it from the callsite_frame at the top of the call site
stack.
Notes
o Under simple test conditions, the number of call site IDs allocated
can be quite small, small enough to fit in 6 bits. That would reduce
the sk_buff growth to one byte. This is *not* a recommended
configuration.
o This is placed in net/core and linux/net since those are the only
current users, but there is nothing about this that is networking-
specific.
Restrictions
o Call site IDs are never reused, so it is possible to exceed the
maximum number of IDs by having a large number of call locations.
In addition, it does not recognize that the same module has been
unloaded and reloaded, so calls from the reloaded module will be
assigned new IDs. Detection of incorrect operations on an sk_buff
is not affected by exhaustion of call site IDs, but it may not be
possible to determine the location of the last operation.
CONFIG_DEBUG_SKB_ID_SIZE is set to reduce the sk_buff growth to 16
bits and should handle most cases. It could be made larger to allow
more call site IDs, if necessary.
o The callsite structures for a module will be freed when that module
is unloaded, even though sk_buffs may be using IDs corresponding to
those call sites. To allow useful error reporting, the call site
information in a module being unloaded is copied. If
CONFIG_CALLSITE_TERSE is not enabled and the module that last changed
the sk_buff is no longer loaded, the address of the call site
is no longer valid, so only the function name and offset are printed
if the module is unloaded. If it is loaded, the address is also
reported.
History
v2 Support small callsite IDs and split out out-of-band parameter
parsing.
V1 Initial release
Signed-off-by: David VomLehn <dvomlehn@cisco.com>
---
include/net/callsite-types.h | 160 +++++++++++++++++++
include/net/callsite.h | 208 +++++++++++++++++++++++++
net/core/callsite.c | 354 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 722 insertions(+), 0 deletions(-)
diff --git a/include/net/callsite-types.h b/include/net/callsite-types.h
new file mode 100644
index 0000000..796cfb1
--- /dev/null
+++ b/include/net/callsite-types.h
@@ -0,0 +1,160 @@
+/*
+ * callsite-types.h
+ *
+ * Definitions for tracking sites at which functions are called with low
+ * overhead.
+ *
+ * Copyright (C) 2009 Scientific-Atlanta, Inc.
+ * Copyright (C) 2010 Cisco Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * Author: David VomLehn
+ */
+
+#ifndef _LINUX_NET_CALLSITE_TYPES_H_
+#define _LINUX_NET_CALLSITE_TYPES_H_
+#include <linux/oobparam.h>
+
+/* Pre-defined call site IDs */
+#define CALLSITE_UNSET 0 /* Never set (default) */
+#define CALLSITE_UNKNOWN 1 /* Tried to set, but couldn't */
+#define CALLSITE_START 2 /* First valid call site ID */
+
+#define CALLSITE_MAX_ID_SIZE 16 /* Max # bits in a call site ID */
+
+/**
+ * struct callsite_id - callsite identifier
+ * @id: Unique value assigned to callsite
+ *
+ * This includes the unique ID assigned to the call site and the information
+ * that defines the location of the call site.
+ */
+struct callsite_id {
+ unsigned id:CALLSITE_MAX_ID_SIZE;
+};
+
+/* The id value must be set to CALLSITE_UNSET. This is conveniently defined
+ * to have the value zero, so we don't need to explicitly set it */
+#define CALLSITE_ID_INIT() { \
+ }
+
+/**
+ * struct callsite_where - Location information for loaded callsites
+ * @here: Address of code doing the calling (if terse reporting)
+ * @file: Pointer to file name (if not using terse reporting)
+ * @lineno: Line number in file (if not using terse reporting)
+ */
+struct callsite_where {
+#ifdef CONFIG_CALLSITE_TERSE
+ void *here; /* Address */
+#else
+ const char *file; /* Call location */
+ unsigned short lineno;
+#endif
+};
+
+#ifdef CONFIG_CALLSITE_TERSE
+#define CALLSITE_WHERE_INIT() { \
+ .here = NULL, \
+ }
+#else
+#define CALLSITE_WHERE_INIT() { \
+ .file = __FILE__, \
+ .lineno = __LINE__, \
+ }
+#endif
+
+/**
+ * struct callsite_const - constant per-callsite information
+ * @id: Pointer to the location of the callsite ID
+ * @where: Location information
+ * @module: Pointer to the module om which the callsite exists
+ * @set: Pointer to information that allies to all callsites of this
+ * particular set.
+ */
+struct callsite_static {
+ struct callsite_id *id;
+ struct callsite_where where;
+ struct module *module;
+ struct callsite_set *set;
+};
+
+#define CALLSITE_STATIC_INIT(_id, _set) { \
+ .id = _id, \
+ .where = CALLSITE_WHERE_INIT(), \
+ .module = THIS_MODULE, \
+ .set = _set, \
+ }
+
+/*
+ * callsite_set - information about a set of callsite IDs
+ * @name: Callsite_set name
+ * @width: Number of bits available for callsite ID
+ * Private members:
+ * @warned: Has a warning been printed that no call site ID could
+ * be assigned for this callsite set?
+ * @max_id: The maximim value of a callsite ID. This must fit in
+ * the number of bits allocated to the callsite_id and
+ * must be at least CALLSITE_START.
+ * @next_id: Value of the next callsite ID to give out. Will never
+ * be more than @max_id.
+ * @info: Pointer to callsite_info array.
+ * @lock: Lock that protects the @callsites structure member
+ * @callsite_id_sets: Link to the next callsite_id_set
+ */
+struct callsite_set {
+ const char *name;
+ unsigned int width;
+ /* private */
+ bool warned:1;
+ unsigned int max_id;
+ unsigned int next_id;
+ struct callsite_info *info;
+ spinlock_t lock;
+ struct list_head callsite_id_sets;
+};
+
+#define CALLSITE_SET_INIT(str_name, _varname, _width) { \
+ .name = str_name, \
+ .width = _width, \
+ .lock = __SPIN_LOCK_UNLOCKED((_varname).lock), \
+ .callsite_id_sets = LIST_HEAD_INIT((_varname).callsite_id_sets), \
+}
+
+/**
+ * struct callsite_frame - data in each "frame" of the callsite stack
+ * @id: Callsite ID
+ * @callsite_oobparam: Data for passing out of band parameters
+ *
+ * Data that is stored on the stack each time a call is made. A linked list
+ * of these is constructed on the stack for each task. In effect, these
+ * are "frames" for the stack of call sites
+ */
+struct callsite_frame {
+ struct callsite_id id;
+ OOBPARAM_FRAME(frame);
+};
+#define CALLSITE_FRAME(name) struct callsite_frame name;
+
+/**
+ * struct callsite_top - pointer to the top of the callsite stack
+ * @callsite_top Pointer to the top of the callsite stack
+ */
+struct callsite_top {
+ OOBPARAM_TOP(top);
+};
+#define CALLSITE_TOP(name) struct callsite_top name;
+#endif
diff --git a/include/net/callsite.h b/include/net/callsite.h
new file mode 100644
index 0000000..a355a23
--- /dev/null
+++ b/include/net/callsite.h
@@ -0,0 +1,208 @@
+/*
+ * callsite.h
+ *
+ * Definitions for tracking callers to functions with very low storage
+ * overhead.
+ *
+ * Copyright (C) 2009 Scientific-Atlanta, Inc.
+ * Copyright (C) 2010 Cisco Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * Author: David VomLehn
+ */
+
+#ifndef _LINUX_NET_CALLSITE_H_
+#define _LINUX_NET_CALLSITE_H_
+#include <linux/stringify.h>
+#include <linux/sched.h>
+#include <net/callsite-types.h>
+
+#ifdef CONFIG_CALLSITE
+/* CALLSITE_VARS - macro to define all variables local to a call site
+ * @cs_top: Name of a variable in which to store the value of the top
+ * of the stack
+ * @cs_id: Name of the statically allocated variable in which the call
+ * site ID is stored
+ * @cs_static: Name of the statically allocated structure in which constant
+ * data about the call site is stored
+ * @cs_sf: Name of the &struct callsite_frame variable (allocated
+ * on the stack)
+ * @set: Pointer to the &struct callsite_set for this set of call sites
+ * @top: Pointer to the &struct callsite_top for this thread
+ */
+#define CALLSITE_VARS(cs_top, cs_id, cs_static, cs_sf, set, top) \
+ struct callsite_top *cs_top = (top); \
+ static struct callsite_id cs_id; \
+ static struct callsite_static cs_static = \
+ CALLSITE_STATIC_INIT(&cs_id, (set)); \
+ struct callsite_frame cs_sf
+
+/* Define a macro for declaring the variables */
+#define CALLSITE_DECL(cs_top, cs_id, cs_static, cs_sf, set, top) \
+ CALLSITE_VARS(cs_top, cs_id, cs_static, cs_sf, (set), (top))
+
+/**
+ * CALLSITE_CALL - Push a callsite stack "frame" and call a function
+ * @top: Pointer to a pointer to the first in the list of
+ * &callsite_top "frames
+ * @set: Pointer to a &struct callsite_set
+ * @fn: Function returning a non-void value
+ * @...: Arguments to fn()
+ *
+ * Push a callsite stack "frame" on the stack, call the given function,
+ * and pop the callsite stack frame. Evaluates to the value returned by
+ * the function.
+ */
+#define CALLSITE_CALL(top, set, fn, arg1, ...) ({ \
+ CALLSITE_DECL(_cs_top, _cs_id, _cs_static, \
+ _cs_stackframe, (set), (top)); \
+ typeof((fn)(arg1, ##__VA_ARGS__)) _cs_result; \
+ callsite_push(_cs_top, &_cs_stackframe, &_cs_id, \
+ &_cs_static); \
+ _cs_result = (fn)(arg1, ##__VA_ARGS__); \
+ callsite_pop(_cs_top); \
+ _cs_result; \
+ })
+
+/**
+ * CALLSITE_CALL_VOID - Push a callsite stack "frame" and call a void function
+ * @top: Pointer to a pointer to the first in the list of
+ * callsite_top "frames
+ * @fn: Function of type void
+ * @...: Arguments to fn()
+ *
+ * Push a callsite stack "frame" on the stack, call the given function,
+ * and pop the callsite stack frame.
+ */
+#define CALLSITE_CALL_VOID(top, set, fn, arg1, ...) do { \
+ CALLSITE_DECL(_cs_top, _cs_id, _cs_static, \
+ _cs_stackframe, (set), (top)); \
+ callsite_push(_cs_top, &_cs_stackframe, &_cs_id, \
+ &_cs_static); \
+ (fn)(arg1, ##__VA_ARGS__); \
+ callsite_pop(_cs_top); \
+ } while (0)
+
+#define CALLSITE_CUR(top) \
+ OOBPARAM_CUR(&top->top, struct callsite_frame, frame)
+
+extern void callsite_print_where_by_id(struct callsite_set *cs_set,
+ unsigned int id);
+extern void callsite_assign_id(struct callsite_static *cs_static);
+extern void callsite_remove_module(struct module *module);
+extern int callsite_set_register(struct callsite_set *cs_set);
+
+/**
+ * callsite_set_id - Set the callsite ID if it isn't already set
+ * @id: Pointer to &callsite_id to check and set
+ * @cs_static: Pointer to &struct callsite_static data for this callsite
+ */
+static inline void callsite_set_id(struct callsite_id *id,
+ struct callsite_static *cs_static)
+{
+ if (unlikely(id->id == CALLSITE_UNSET))
+ callsite_assign_id(cs_static);
+}
+
+/*
+ * callsite_push - Push the current callsite on the callsite stack
+ * @top: Pointer to the stack information
+ * @s: Pointer to stack "frame" on stack.
+ * @cs_where: Pointer to statically allocated per-callsite location
+ * information
+ */
+static inline void callsite_push(struct callsite_top *top,
+ struct callsite_frame *s, struct callsite_id *id,
+ struct callsite_static *cs_static)
+{
+ callsite_set_id(id, cs_static);
+ s->id = *id;
+ oobparam_push(&top->top, &s->frame);
+}
+
+/*
+ * callsite_pop - Pop the current callsite from the callsite stack
+ * @top: Pointer to a pointer to the top of the callsite stack
+ *
+ * It is possible that the memory pointed to by top will be reused once it
+ * goes out of scope, and the storage now used by the next element of
+ * the top callsite_top structure modified. If top has not been changed
+ * by then, the linked list will be thoroughly confused. We use barrier() to
+ * ensure that top is changed before the callsite_top structure goes out
+ * of scope.
+ */
+static inline void callsite_pop(struct callsite_top *top)
+{
+ oobparam_pop(&top->top);
+}
+
+/**
+ * callsite_top_id - Get the &callsite_id for the topmost &callsite_frame
+ * @top: Pointer to the &struct callsite_top
+ */
+static inline int callsite_get_id(struct callsite_top *top)
+{
+ return CALLSITE_CUR(top)->id.id;
+}
+
+/**
+ * callsite_task_init - initialize a callsite member of the task structure
+ * @p Pointer to the member to initialize
+ */
+static inline void callsite_top_init(struct callsite_top *p)
+{
+}
+#else
+#define CALLSITE_CALL(top, set, fn, arg1, ...) ({ \
+ (fn)(arg1, ##__VA_ARGS__); \
+ })
+
+/* Macro to use to call functions that do not return values */
+#define CALLSITE_CALL_VOID(top, set, fn, arg1, ...) do { \
+ (fn)(arg1, ##__VA_ARGS__); \
+ } while (0)
+
+static inline void callsite_remove_module(struct module *module)
+{
+}
+
+static inline void callsite_push(struct callsite_top *top,
+ struct callsite_frame *s, struct callsite_id *id,
+ const struct callsite_static *cs_static)
+{
+}
+
+static inline void callsite_pop(struct callsite_top **top)
+{
+}
+
+static inline void callsite_top_init(struct callsite_top *p)
+{
+}
+
+static inline int callsite_set_register(struct callsite_set *cs_set)
+{
+ return 0;
+}
+
+static inline int callsite_top_id(const struct callsite_frame *top)
+{
+ return 0;
+}
+#endif
+
+extern void *here(void);
+#endif
diff --git a/net/core/callsite.c b/net/core/callsite.c
new file mode 100644
index 0000000..e77d44b
--- /dev/null
+++ b/net/core/callsite.c
@@ -0,0 +1,354 @@
+/*
+ * callsite.c
+ *
+ * Support for assigning IDs to addresses.
+ *
+ * For debugging, it may be desirable to store information about where a
+ * structure was, for example used or modified. This location would generically
+ * be an address, but since there are generally only a small number of
+ * addresses that would actually be used, a much smaller tag value can be
+ * stored instead. This code takes care of dynamically generating IDs and
+ * converting them to addresses, when necessary. It is written with the
+ * assumption that generating IDs must be very fast, but converting them
+ * to addresses does not need to be particularly quick.
+ *
+ * Copyright (C) 2009 Scientific-Atlanta, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * Author: David VomLehn
+ */
+
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/kallsyms.h>
+#include <linux/module.h>
+#include <net/callsite.h>
+
+/**
+ * struct callsite_info - Per-call site information
+ * @unloaded: Is this in a module that has been unloaded?
+ * @where: Union of location information
+ * @loaded Location information if callsite is loaded
+ * @unloaded Location information if callsite is in an unloaded module
+ * @lock: Lock for updating information for this call site
+ */
+struct callsite_info {
+ bool unloaded:1;
+ union {
+ const struct callsite_static *loaded;
+ const char *unloaded;
+ } where;
+};
+
+/* List of callsite_id_sets and the synchronization primitive that protects
+ * the list */
+static DEFINE_MUTEX(callsite_id_sets_lock);
+static LIST_HEAD(callsite_id_sets);
+
+static const char unknown_location[] = "<unknown>";
+
+/**
+ * callsite_print_location - Print location from &struct callsite_where
+ * @where: Pointer to &struct callsite_where to print
+ *
+ * The caller of this function should have already printed the printk()
+ * priority so that these pr_cont()s will use the same priority.
+ */
+#ifdef CONFIG_CALLSITE_TERSE
+void callsite_print_where(const struct callsite_where *where)
+{
+ print_ip_sym((unsigned long) where->here);
+}
+#else
+void callsite_print_where(const struct callsite_where *where)
+{
+ pr_cont("%s:%u\n", where->file, where->lineno);
+}
+#endif
+
+/**
+ * callsite_location_alloc - Store location in allocated string
+ * @cs: Pointer to the &struct callsite
+ *
+ * Returns a pointer to an allocated string, or %NULL.
+ */
+#ifdef CONFIG_CALLSITE_TERSE
+static char *callsite_location_alloc(const struct callsite_where *where)
+{
+ char *location;
+
+ location = kmalloc(KSYM_SYMBOL_LEN, GFP_KERNEL);
+
+ /* The only interesting information is really the function and offset
+ * information. The actual location is irrelevant because the module
+ * is going away. */
+ if (location != NULL) {
+ size_t size;
+ snprintf(location, KSYM_SYMBOL_LEN, "%pS", where->here);
+ size = strlen(location) + 1;
+ location = krealloc(location, size, GFP_KERNEL);
+ }
+
+ return location;
+}
+#else
+static char *callsite_location_alloc(const struct callsite_where *where)
+{
+ char *location;
+ size_t size;
+ static const char colon[] = ":";
+ static const char lineno[] = "99999";
+
+ size = strlen(where->file) + sizeof(colon) + sizeof(lineno) + 1;
+ location = kmalloc(size, GFP_KERNEL);
+
+ if (location != NULL)
+ snprintf(location, size, "%s:%u", where->file, where->lineno);
+
+ return location;
+}
+#endif
+
+/**
+ * callsite_location - Get a string for the location
+ * @cs: Pointer to call site information
+ */
+static const char *callsite_location(const struct callsite_where *where)
+{
+ const char *p;
+
+ p = callsite_location_alloc(where);
+ if (p == NULL)
+ p = unknown_location;
+ return p;
+}
+
+/**
+ * callsite_print_location_by_id - Symbolically print the caller location
+ * @cs_set: Pointer to information about this set of caller IDs
+ * @id: Call site ID to search for
+ *
+ * The caller of this function should have already printed the printk()
+ * priority so that these pr_cont()s will use the same priority.
+ */
+void callsite_print_where_by_id(struct callsite_set *cs_set,
+ unsigned int id)
+{
+ struct callsite_info *p;
+ unsigned long flags;
+
+ switch (id) {
+ case CALLSITE_UNSET:
+ pr_cont("<unset>\n");
+ break;
+ case CALLSITE_UNKNOWN:
+ pr_cont("%s\n", unknown_location);
+ break;
+ default:
+ spin_lock_irqsave(&cs_set->lock, flags);
+
+ /* If the ID is not valid, we can't say where what the callsite
+ * might be. */
+ if (id >= cs_set->next_id || cs_set->info == NULL)
+ pr_cont("<unknown ID %u>\n", id); /* Couldn't find ID */
+
+ else {
+ p = &cs_set->info[id - CALLSITE_START];
+
+ if (p->unloaded)
+ pr_cont("%s (module unloaded)\n",
+ p->where.unloaded);
+ else
+ callsite_print_where(&p->where.loaded->where);
+ }
+
+ spin_unlock_irqrestore(&cs_set->lock, flags);
+ break;
+ }
+}
+
+/**
+ * callsite_assign_id - Assign a call site ID
+ * @cs_static: Pointer to static information about the callsite
+ *
+ * If the ID is @CALLSITE_UNSET in a given &struct callsite, this
+ * function is called to assign a call site ID. The value assigned will
+ * normally * be @CALLSITE_START or above, but if we exceed the maximum
+ * size of an ID, * we assign @CALLSITE_UNKNOWN.
+ */
+extern void callsite_assign_id(struct callsite_static *cs_static)
+{
+ unsigned int id;
+ unsigned long flags;
+ struct callsite_set *set;
+
+ /* Record the caller's location. */
+ cs_static->where.here = here();
+
+ /* Lock the call site set. The first time we check, we do so on
+ * the optimistic assumption that it has already been set. It may
+ * have been since checking, though, which is why we need to lock
+ * the callsite_set and check again. */
+ set = cs_static->set;
+ spin_lock_irqsave(&set->lock, flags);
+
+ /* If the callsite_info array wasn't allocated, we can't assign an
+ * ID and the callsite is unknown. Since the value returned is not
+ * @CALLSITE_UNKNOWN, we won't try again to assign a callsite ID for
+ * this site, */
+ if (set->info == NULL)
+ cs_static->id->id = CALLSITE_UNKNOWN;
+
+ else if (cs_static->id->id == CALLSITE_UNSET) {
+
+ /* If we are out of tags, just indicate that it's unknown */
+ if (set->next_id > set->max_id) {
+ id = CALLSITE_UNKNOWN;
+ if (!set->warned) {
+ pr_warning("Exhausted IDs for callsite_set "
+ "%s\n", set->name);
+ set->warned = true;
+ }
+ }
+
+ else {
+ struct callsite_info *p;
+ id = set->next_id;
+ set->next_id++;
+
+ p = &set->info[id - CALLSITE_START];
+ p->unloaded = false;
+ p->where.loaded = cs_static;
+ }
+
+ cs_static->id->id = id;
+ }
+
+ spin_unlock_irqrestore(&set->lock, flags);
+#ifdef DEBUG
+ if (cs_static->id->id != CALLSITE_UNKNOWN) {
+ pr_debug("%s: assigned ID %u to call at ",
+ __func__, cs_static->id->id);
+ callsite_print_where_by_id(set, cs_static->id->id);
+ }
+#endif
+}
+EXPORT_SYMBOL(callsite_assign_id);
+
+/*
+ * callsite_unload_id - Preserve call site ID info on module unload
+ * @p: Pointer to &struct callsite_info to preserve
+ *
+ * We assume that we are not in atomic mode, so we can sleep waiting for
+ * memory. Must be called with the list lock held.
+ */
+static void callsite_unload_id(struct callsite_info *p)
+{
+ p->unloaded = true;
+ p->where.unloaded = callsite_location(&p->where.loaded->where);
+}
+
+/*
+ * callsite_unload_module - Preserve callsite ID info when unloading a module
+ * @module: Pointer to &struct module for module being unloaded
+ *
+ * Call site IDs are assigned dynamically as the need arises, which works well
+ * much of the time. There is an issue, though, with call site ID information
+ * stored in modules, because the callsite associated with an ID
+ * goes away when that module is removed. To handle that, we copy all of the
+ * callsite information for a module when it is removed, including
+ * generating the location string.
+ */
+void callsite_remove_module(struct module *module)
+{
+ unsigned long flags;
+ struct callsite_set *cs;
+
+ mutex_lock(&callsite_id_sets_lock);
+ list_for_each_entry(cs, &callsite_id_sets, callsite_id_sets) {
+ int i;
+
+ if (cs->info == NULL)
+ continue;
+
+ spin_lock_irqsave(&cs->lock, flags);
+
+ for (i = CALLSITE_START; i < cs->next_id; i++) {
+ struct callsite_info *p;
+
+ p = &cs->info[i - CALLSITE_START];
+
+ if (!p->unloaded && p->where.loaded->module == module)
+ callsite_unload_id(p);
+ }
+
+ spin_unlock_irqrestore(&cs->lock, flags);
+ }
+ mutex_unlock(&callsite_id_sets_lock);
+}
+
+/**
+ * callsite_id_set_register - add information for a set of callsite IDs
+ * @cs_set: Pointer to the &struct callsite_id_set to add
+ *
+ * Returns zero on success, or a negative errno value.
+ */
+int callsite_set_register(struct callsite_set *cs_set)
+{
+ size_t n;
+ size_t size;
+ struct callsite_info *info;
+
+ BUG_ON(cs_set->max_id > (1 << CALLSITE_MAX_ID_SIZE));
+ cs_set->max_id = (1 << (cs_set->width)) - 1;
+
+ if (cs_set->max_id < CALLSITE_START)
+ return -EINVAL;
+
+ n = cs_set->max_id - CALLSITE_START;
+
+ /* Allocate memory for the maximum number of callsites. We take
+ * advantage of the fact that the value of CALLSITE_UNSET is zero */
+ size = n * sizeof(struct callsite_info);
+ BUG_ON(CALLSITE_UNSET != 0);
+ info = kzalloc(size, GFP_KERNEL);
+
+ if (info == NULL)
+ return -ENOMEM;
+
+ cs_set->info = info;
+ cs_set->next_id = CALLSITE_START;
+
+ mutex_lock(&callsite_id_sets_lock);
+ list_add(&cs_set->callsite_id_sets, &callsite_id_sets);
+ mutex_unlock(&callsite_id_sets_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(callsite_set_register);
+
+/**
+ * here - address in calling function
+ *
+ * This needs the caller to create a stackframe, so it can't be inlined.
+ */
+noinline void *here()
+{
+ return __builtin_return_address(0);
+}
+EXPORT_SYMBOL(here);
^ permalink raw reply related
* [PATCH][RFC] Check sk_buff states
From: David VomLehn @ 2010-06-08 0:30 UTC (permalink / raw)
To: to, "netdev, netdev
This uses the oobparam and callsite infrastructure to implement sk_buff
state checking and error reporting. Possible states of an sk_buff are:
SKB_STATE_FREE - Is not currently in use
SKB_STATE_ALLOCATED - Has been allocated, but is not on a queue
SKB_STATE_QUEUED - Is allocated and on a queue
Since there are only three states, two bits suffice to record the state of
an sk_buff structure, so checking for consistent state is easy. (For you
weenies, the fourth possible state *is* flagged as an error).
Some of the errors that can be detected are:
o Double-freeing an skb_buff
o Using kfree() instead of kfree_skb()
o Queuing an sk_buffer that is already on a queue
Notes
o Under relatively simple test conditions, .i.e. one Ethernet interface
using an NFS filesystem, the number of call site IDs allocated
can be quite small, small enough to fit in 6 bits. That would reduce
the sk_buff growth to one byte. This is *not* a recommended
configuration.
Restrictions
o This code relies on fact that sk_buff memory is allocated from
dedicated kmem_caches. Specifically:
1. It uses kmem_cache constructors to initialize fields used for
checking state.
2. It assumes that the fields used to check state will either:
a. Not be modified between calling kmem_cache_free and
kmem_cache_alloc_node, or
b. The kmem_cache constructor, will be called.
o Since the state and callsite ID must not be zeroed by __alloc_skb(),
it may not be possible to squeeze them into existing padding in
the sk_buff structure. If so, the addition size added might be larger
than the 12 bits presently configured.
o There is some chance that use of kfree() instead of skb_free() will
not be detected. Normally, when the skbuff freed with kfree() is
reallocated, its state will still be SKB_STATE_ALLOCATED. Since
alloc_skb() expects the state to be SKB_STATE_FREE, the error will
be caught. However, if the slab used for the skbuff gets returned
to the buddy allocator and then reallocated to the kmem_cache,
it will get re-initialized. This will set its state to SKB_STATE_FREE.
Returning a slab to the buddy allocator is done much less frequently
than allocation and freeing, so the bulk of these errors will be
found.
o Though this code should not have much of a performance or size
impact, careful consideration should be given before enabling it
in a production version of the kernel.
History
v2 Reduce the per-sk_buff overhead by shrinking the state and using
call site IDs. Instead of storing the last allocation and free, only
store the last state-changing operation.
v1 Original version
Signed-off-by: David VomLehn <dvomlehn@cisco.com>
---
include/linux/sched.h | 5 +
include/linux/skbuff.h | 279 +++++++++++++++++++++++++++++++++++++++++++++++-
kernel/fork.c | 3 +
kernel/module.c | 2 +
lib/Kconfig.debug | 53 +++++++++
net/core/Makefile | 2 +-
net/core/skbuff.c | 188 ++++++++++++++++++++++++++++++++-
7 files changed, 523 insertions(+), 9 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f118809..e6a39e1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -91,6 +91,7 @@ struct sched_param {
#include <linux/kobject.h>
#include <linux/latencytop.h>
#include <linux/cred.h>
+#include <net/callsite-types.h>
#include <asm/processor.h>
@@ -1389,6 +1390,10 @@ struct task_struct {
int softirqs_enabled;
int softirq_context;
#endif
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+ /* Stack of calls using callsite ID logging */
+ CALLSITE_TOP(skb_callsite_top);
+#endif
#ifdef CONFIG_LOCKDEP
# define MAX_LOCK_DEPTH 48UL
u64 curr_chain_key;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index bf243fc..0bd52ea 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -29,6 +29,7 @@
#include <linux/rcupdate.h>
#include <linux/dmaengine.h>
#include <linux/hrtimer.h>
+#include <net/callsite.h>
/* Don't change this without changing skb_csum_unnecessary! */
#define CHECKSUM_NONE 0
@@ -88,7 +89,7 @@
* about CHECKSUM_UNNECESSARY. 8)
* NETIF_F_IPV6_CSUM about as dumb as the last one but does IPv6 instead.
*
- * Any questions? No questions, good. --ANK
+ * Any questions? No questions, good. --ANK
*/
struct net_device;
@@ -254,6 +255,32 @@ typedef unsigned int sk_buff_data_t;
typedef unsigned char *sk_buff_data_t;
#endif
+/* States for a struct sk_buff */
+enum skb_check_state {
+ SKB_STATE_INVALID, /* Zero is a common corruption value, so make
+ * an invalid value */
+ SKB_STATE_FREE, /* sk_buff is not allocated */
+ SKB_STATE_ALLOCATED, /* In used, but not queued */
+ SKB_STATE_QUEUED, /* On a queue */
+ SKB_STATE_NUM /* Number of states */
+};
+
+#define SKB_VALIDATE_STATE_SIZE 2 /* ilog2(SKB_STATE_NUM) */
+
+/*
+ * Convert an skb_check to the corresponding mask
+ * @state: The skb_check value to convert
+ * Returns the mask
+ */
+static inline unsigned skb_check2mask(enum skb_check_state state)
+{
+ return 1u << state;
+}
+
+#define SKB_STATE_FREE_MASK (1 << SKB_STATE_FREE)
+#define SKB_STATE_ALLOCATED_MASK (1 << SKB_STATE_ALLOCATED)
+#define SKB_STATE_QUEUED_MASK (1 << SKB_STATE_QUEUED)
+
/**
* struct sk_buff - socket buffer
* @next: Next buffer in list
@@ -308,6 +335,8 @@ typedef unsigned char *sk_buff_data_t;
* done by skb DMA functions
* @secmark: security marking
* @vlan_tci: vlan tag control information
+ * @state: State for consistency checking
+ * @last_op: Address of caller who last changed the sk_buff state
*/
struct sk_buff {
@@ -409,6 +438,14 @@ struct sk_buff {
*data;
unsigned int truesize;
atomic_t users;
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+ /* private: */
+ /* These are set by the constructor called by the slab cache allocator
+ * and should not be zeroed with the rest of the sk_buff. */
+ unsigned int state:SKB_VALIDATE_STATE_SIZE;
+ unsigned int last_op:CONFIG_DEBUG_SKB_ID_WIDTH;
+ /* public: */
+#endif
};
#ifdef __KERNEL__
@@ -484,6 +521,130 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
return (struct rtable *)skb_dst(skb);
}
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+extern struct callsite_set skb_callsite_set;
+
+extern void skb_report_invalid(const struct sk_buff *skb, unsigned expected);
+
+static inline void skb_callsite_task_init(struct task_struct *p)
+{
+ callsite_top_init(&p->skb_callsite_top);
+}
+
+/**
+* skb_check - verify that the state of the sk_buff is as expected
+* @skb: Pointer to the &struct sk_buff to validate
+* @expected: Mask of valid states
+*/
+static inline void skb_check(const struct sk_buff *skb, unsigned expected)
+{
+ if (unlikely((skb_check2mask(skb->state) & expected) == 0))
+ skb_report_invalid(skb, expected);
+}
+
+/**
+* skb_check_and_set - validate the current sk_buff state and set to a new value
+* @skb: Pointer to the &sk_buff whose state is to be validated and set
+* @expected: Expected state of the &sk_buff
+* @new_state: New state of the &sk_buff
+*/
+static inline void skb_check_and_set(struct sk_buff *skb, unsigned expected,
+ enum skb_check_state new_state)
+{
+ skb_check(skb, expected);
+ skb->state = new_state;
+}
+
+/**
+ * skb_check_and_set_state - Check and set the state for specific function
+ * @skb: Pointer to the &sk_buff whose state is to be validated and set
+ * @expected: Expected state mask for the &sk_buff
+ * @state: New state for the sk_buff
+ */
+static inline void skb_check_and_set_state(struct sk_buff *skb,
+ unsigned expected, enum skb_check_state state)
+{
+ skb_check_and_set(skb, expected, state);
+ skb->last_op = callsite_get_id(¤t->skb_callsite_top);
+}
+
+/**
+ * skb_check_and_set_alloc - Check and set the state for allocation function
+ * @skb: Pointer to the &sk_buff whose state is to be validated and set
+ * @expected: Expected state mask for the &sk_buff
+ *
+ * We allow %NULL values for skb, in which case we do nothing.
+ */
+static inline void skb_check_and_set_alloc(struct sk_buff *skb,
+ unsigned expected)
+{
+ if (likely(skb))
+ skb_check_and_set_state(skb, expected, SKB_STATE_ALLOCATED);
+}
+
+/**
+ * skb_check_and_set_free - Check and set the state for free function
+ * @skb: Pointer to the &sk_buff whose state is to be validated and set
+ * @expected: Expected state mask for the &sk_buff
+ *
+ * %NULL values for skb are valid in this case, which causes the check to be
+ * skipped.
+ */
+static inline void skb_check_and_set_free(struct sk_buff *skb,
+ unsigned expected)
+{
+ if (likely(skb))
+ skb_check_and_set_state(skb, expected, SKB_STATE_FREE);
+}
+
+/**
+ * skb_check_and_set_queue - Check and set the state for queuing functions
+ * @skb: Pointer to the &sk_buff whose state is to be validated and set
+ * @expected: Expected state mask for the &sk_buff
+ *
+ * %NULL values for skb are valid in this case, which causes the check to be
+ * skipped.
+ */
+static inline void skb_check_and_set_queued(struct sk_buff *skb,
+ unsigned expected)
+{
+ skb_check_and_set_state(skb, expected, SKB_STATE_QUEUED);
+}
+#else
+static inline void skb_callsite_task_init(struct task_struct *p)
+{
+}
+
+static inline void skb_check(const struct sk_buff *skb, unsigned expected)
+{
+}
+
+static inline void skb_check_and_set(struct sk_buff *skb, unsigned expected,
+ const enum skb_check_state new_state)
+{
+}
+
+static inline void skb_check_and_set_state(struct sk_buff *skb,
+ unsigned expected, enum skb_check_state state)
+{
+}
+
+static inline void skb_check_and_set_alloc(struct sk_buff *skb,
+ unsigned expected)
+{
+}
+
+static inline void skb_check_and_set_free(struct sk_buff *skb,
+ unsigned expected)
+{
+}
+
+static inline void skb_check_and_set_queued(struct sk_buff *skb,
+ unsigned expected)
+{
+}
+#endif
+
extern void kfree_skb(struct sk_buff *skb);
extern void consume_skb(struct sk_buff *skb);
extern void __kfree_skb(struct sk_buff *skb);
@@ -886,6 +1047,7 @@ static inline void __skb_insert(struct sk_buff *newsk,
struct sk_buff *prev, struct sk_buff *next,
struct sk_buff_head *list)
{
+ skb_check_and_set_queued(newsk, SKB_STATE_ALLOCATED_MASK);
newsk->next = next;
newsk->prev = prev;
next->prev = prev->next = newsk;
@@ -1040,6 +1202,8 @@ static inline void __skb_unlink(struct sk_buff *skb, struct sk_buff_head *list)
{
struct sk_buff *next, *prev;
+ skb_check_and_set_state(skb, SKB_STATE_QUEUED_MASK,
+ SKB_STATE_ALLOCATED);
list->qlen--;
next = skb->next;
prev = skb->prev;
@@ -1116,8 +1280,8 @@ static inline void skb_fill_page_desc(struct sk_buff *skb, int i,
extern void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page,
int off, int size);
-#define SKB_PAGE_ASSERT(skb) BUG_ON(skb_shinfo(skb)->nr_frags)
-#define SKB_FRAG_ASSERT(skb) BUG_ON(skb_has_frags(skb))
+#define SKB_PAGE_ASSERT(skb) BUG_ON(skb_shinfo(skb)->nr_frags)
+#define SKB_FRAG_ASSERT(skb) BUG_ON(skb_has_frags(skb))
#define SKB_LINEAR_ASSERT(skb) BUG_ON(skb_is_nonlinear(skb))
#ifdef NET_SKBUFF_DATA_USES_OFFSET
@@ -1554,9 +1718,9 @@ extern struct page *__netdev_alloc_page(struct net_device *dev, gfp_t gfp_mask);
* netdev_alloc_page - allocate a page for ps-rx on a specific device
* @dev: network device to receive on
*
- * Allocate a new page node local to the specified device.
+ * Allocate a new page node local to the specified device.
*
- * %NULL is returned if there is no free memory.
+ * %NULL is returned if there is no free memory.
*/
static inline struct page *netdev_alloc_page(struct net_device *dev)
{
@@ -2144,5 +2308,110 @@ static inline void skb_forward_csum(struct sk_buff *skb)
}
bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off);
+
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+/*
+ * Macros used to call functions that record the location of the last operation
+ * performed on an sk_buff. We use these instead of using the CALLSITE_CALL*
+ * macros directly so that we can redefine them in skbuff.c
+ */
+#define SKB_CALL(fn, arg1, ...) ({ \
+ CALLSITE_CALL(¤t->skb_callsite_top, \
+ &skb_callsite_set, fn, arg1, ##__VA_ARGS__); \
+ })
+
+#define SKB_CALL_VOID(fn, arg1, ...) do { \
+ CALLSITE_CALL_VOID(¤t->skb_callsite_top, \
+ &skb_callsite_set, fn, arg1, ##__VA_ARGS__); \
+ } while (0)
+/*
+ * Macros that cover functions that call callsite_get_id() directly or
+ * indirectly. The macros must be defined after all of the functions are
+ * defined so that we don't have a covered function calling a covered function.
+ */
+#define kfree_skb(skb) \
+ SKB_CALL_VOID(kfree_skb, skb)
+#define consume_skb(skb) \
+ SKB_CALL_VOID(consume_skb, skb)
+#define __kfree_skb(skb) \
+ SKB_CALL_VOID(__kfree_skb, skb)
+#define __alloc_skb(size, priority, fclone, node) \
+ SKB_CALL(__alloc_skb, size, priority, fclone, node)
+#define alloc_skb(size, priority) \
+ SKB_CALL(alloc_skb, size, priority)
+#define skb_recycle_check(skb, skb_size) \
+ SKB_CALL(skb_recycle_check, skb, skb_size)
+#define alloc_skb_fclone(size, priority) \
+ SKB_CALL(alloc_skb_fclone, size, priority)
+#define skb_morph(dst, src) \
+ SKB_CALL(skb_morph, dst, src)
+#define skb_clone(skb, priority) \
+ SKB_CALL(skb_clone, skb, priority)
+#define skb_copy(skb, priority) \
+ SKB_CALL(skb_copy, skb, priority)
+#define pskb_copy(skb, gfp_mask) \
+ SKB_CALL(pskb_copy, skb, gfp_mask)
+#define skb_realloc_headroom(skb, headroom) \
+ SKB_CALL(skb_realloc_headroom, skb, headroom)
+#define skb_copy_expand(skb, newheadroom, newtailroom, priority) \
+ SKB_CALL(skb_copy_expand, skb, newheadroom, newtailroom, priority)
+#define skb_cow_data(skb, tailbits, trailer) \
+ SKB_CALL(skb_cow_data, skb, tailbits, trailer)
+#define skb_share_check(skb, pri) \
+ SKB_CALL(skb_share_check, skb, pri)
+#define skb_unshare(skb, pri) \
+ SKB_CALL(skb_unshare, skb, pri)
+#define __pskb_pull_tail(skb, delta) \
+ SKB_CALL(__pskb_pull_tail, skb, delta)
+#define ___pskb_trim(skb, len) \
+ SKB_CALL(___pskb_trim, skb, len)
+#define __pskb_trim(skb, len) \
+ SKB_CALL(__pskb_trim, skb, len)
+#define pskb_trim(skb, len) \
+ SKB_CALL(pskb_trim, skb, len)
+#define skb_queue_purge(list) \
+ SKB_CALL_VOID(skb_queue_purge, list)
+#define __skb_queue_purge(list) \
+ SKB_CALL_VOID(__skb_queue_purge, list)
+#define __dev_alloc_skb(length, gfp_mask) \
+ SKB_CALL(__dev_alloc_skb, length, gfp_mask)
+#define dev_alloc_skb(length) \
+ SKB_CALL(dev_alloc_skb, length)
+#define __netdev_alloc_skb(dev, length, gfp_mask) \
+ SKB_CALL(__netdev_alloc_skb, dev, length, gfp_mask)
+#define netdev_alloc_skb(dev, length) \
+ SKB_CALL(netdev_alloc_skb, dev, length)
+#define skb_segment(skb, features) \
+ SKB_CALL(skb_segment, skb, features)
+#define nf_conntrack_put_reasm(skb) \
+ SKB_CALL(nf_conntrack_put_reasm, skb)
+#define __skb_insert(newsk, prev, next, list) \
+ SKB_CALL_VOID(__skb_insert, newsk, prev, next, list)
+#define __skb_queue_after(list, prev, newsk) \
+ SKB_CALL_VOID(__skb_queue_after, list, prev, newsk)
+#define __skb_queue_before(list, prev, newsk) \
+ SKB_CALL_VOID(__skb_queue_before, list, prev, newsk)
+#define skb_queue_head(list, newsk) \
+ SKB_CALL_VOID(skb_queue_head, list, newsk)
+#define __skb_queue_head(list, newsk) \
+ SKB_CALL_VOID(__skb_queue_head, list, newsk)
+#define skb_queue_tail(list, newsk) \
+ SKB_CALL_VOID(skb_queue_tail, list, newsk)
+#define __skb_queue_tail(list, newsk) \
+ SKB_CALL_VOID(__skb_queue_tail, list, newsk)
+#define skb_unlink(skb, list) \
+ SKB_CALL_VOID(skb_unlink, skb, list)
+#define __skb_unlink(skb, list) \
+ SKB_CALL_VOID(__skb_unlink, skb, list)
+#define skb_dequeue(list) \
+ SKB_CALL(skb_dequeue, list)
+#define __skb_dequeue(list) \
+ SKB_CALL(__skb_dequeue, list)
+#define skb_dequeue_tail(list) \
+ SKB_CALL(skb_dequeue_tail, list)
+#define __skb_dequeue_tail(list) \
+ SKB_CALL(__skb_dequeue_tail, list)
+
+#endif /* CONFIG_DEBUG_SKB_STATE_CHANGES */
#endif /* __KERNEL__ */
#endif /* _LINUX_SKBUFF_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index b6cce14..13b90a5 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1093,6 +1093,9 @@ static struct task_struct *copy_process(unsigned long clone_flags,
#else
p->hardirqs_enabled = 0;
#endif
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+ skb_callsite_task_init(p);
+#endif
p->hardirq_enable_ip = 0;
p->hardirq_enable_event = 0;
p->hardirq_disable_ip = _THIS_IP_;
diff --git a/kernel/module.c b/kernel/module.c
index 8c6b428..7e4d615 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -55,6 +55,7 @@
#include <linux/async.h>
#include <linux/percpu.h>
#include <linux/kmemleak.h>
+#include <net/callsite.h>
#define CREATE_TRACE_POINTS
#include <trace/events/module.h>
@@ -788,6 +789,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
/* Store the name of the last unloaded module for diagnostic purposes */
strlcpy(last_unloaded_module, mod->name, sizeof(last_unloaded_module));
ddebug_remove_module(mod->name);
+ callsite_remove_module(mod);
free_module(mod);
return 0;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e722e9d..27a9a3e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -315,6 +315,59 @@ config DEBUG_OBJECTS_ENABLE_DEFAULT
help
Debug objects boot parameter default value
+config DEBUG_SKB_STATE_CHANGES
+ bool "Validate state of networking buffers (struct sk_buff)"
+ depends on DEBUG_KERNEL && NET
+ default n
+ select CALLSITE
+ help
+ Simple and quick checks to validate that sk_buffs are in the right
+ state. Intended to catch errors, including, but not limited to,
+ the following:
+ - Double-freeing sk_buffs
+ - Freeing sk_buffs that are still on a queue
+ - Using kfree instead of kfree_skb (might miss some instances)
+
+config DEBUG_SKB_ID_WIDTH
+ int "Maximum number of bits used to track sk_buff call sites"
+ depends on DEBUG_SKB_STATE_CHANGES
+ range 6 16
+ default 10
+ help
+ The sk_buff state validation code tracks the location of the last
+ state-changing operation within each sk_buff. To minimize memory
+ usage, it stores IDs instead of the address. This value specifies
+ the maximum number of bits allocated for the ID. If this value is
+ too small, sk_buff state validation is still done, but it will not
+ be possible to determine the location where the state was last
+ changed.
+
+ If in doubt, leave this at its default setting.
+
+config CALLSITE
+ bool
+ help
+ Enable code that dynamically generates small tags from code
+ addresses, which allows reduced overhead in structures that need
+ to store call location information. This is typically used for
+ track sites at which calls are made for debugging.
+
+config CALLSITE_TERSE
+ bool "Use terse reporting of call sites"
+ default "n"
+ depends on DEBUG_KERNEL
+ help
+ When call site IDs are being used to store references to locations
+ where specific functions are called, the call sites can be reported
+ either as a file name/line number pair or using the "%pS" format.
+ Although using file name and line numbers is more readable, it takes
+ significantly more space. The "%pS" format will report the location
+ corresponding to the start of the line where the call to the skb_*
+ function is located. There are generally multiple instructions that
+ must be skipped to find the actual location of the call, which may
+ actually have been expanded inline. Thus, using this option is only
+ for those comfortable with looking at disassembled C code.
+
config DEBUG_SLAB
bool "Debug slab memory allocations"
depends on DEBUG_KERNEL && SLAB && !KMEMCHECK
diff --git a/net/core/Makefile b/net/core/Makefile
index 51c3eec..8cbb6f8 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -18,4 +18,4 @@ obj-$(CONFIG_NET_DMA) += user_dma.o
obj-$(CONFIG_FIB_RULES) += fib_rules.o
obj-$(CONFIG_TRACEPOINTS) += net-traces.o
obj-$(CONFIG_NET_DROP_MONITOR) += drop_monitor.o
-
+obj-$(CONFIG_CALLSITE) += callsite.o
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9f07e74..f320ea9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -57,12 +57,14 @@
#include <linux/init.h>
#include <linux/scatterlist.h>
#include <linux/errqueue.h>
+#include <linux/kallsyms.h>
#include <net/protocol.h>
#include <net/dst.h>
#include <net/sock.h>
#include <net/checksum.h>
#include <net/xfrm.h>
+#include <net/callsite.h>
#include <asm/uaccess.h>
#include <asm/system.h>
@@ -70,6 +72,21 @@
#include "kmap_skb.h"
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+/*
+ * In this file, we never want to use the macros that use the CALLSITE_CALL*
+ * macros to call functions. Doing so would interpose a callsite_stackframe
+ * between the user's call and the call in this file, which would cause
+ * an uninteresting callsite to be recorded. Instead, we define macros that
+ * just call the functions.
+ */
+#undef SKB_CALL
+#define SKB_CALL(fn, arg1, ...) fn(arg1, ##__VA_ARGS__)
+
+#undef SKB_CALL_VOID
+#define SKB_CALL_VOID(fn, arg1, ...) fn(arg1, ##__VA_ARGS__)
+#endif
+
static struct kmem_cache *skbuff_head_cache __read_mostly;
static struct kmem_cache *skbuff_fclone_cache __read_mostly;
@@ -193,7 +210,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
/*
* Only clear those fields we need to clear, not those that we will
* actually initialise below. Hence, don't put any more fields after
- * the tail pointer in struct sk_buff!
+ * the tail pointer in struct sk_buff! (Unless you don't *want* them
+ * to be cleared, such as with the sk_buff validation fields)
*/
memset(skb, 0, offsetof(struct sk_buff, tail));
skb->truesize = size + sizeof(struct sk_buff);
@@ -224,6 +242,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
child->fclone = SKB_FCLONE_UNAVAILABLE;
}
+ skb_check_and_set_alloc(skb, SKB_STATE_FREE_MASK);
out:
return skb;
nodata:
@@ -312,6 +331,7 @@ static void skb_drop_list(struct sk_buff **listp)
do {
struct sk_buff *this = list;
list = list->next;
+ skb_check_and_set_queued(this, SKB_STATE_QUEUED_MASK);
kfree_skb(this);
} while (list);
}
@@ -357,11 +377,14 @@ static void kfree_skbmem(struct sk_buff *skb)
switch (skb->fclone) {
case SKB_FCLONE_UNAVAILABLE:
+ skb_check_and_set_free(skb, SKB_STATE_ALLOCATED_MASK);
kmem_cache_free(skbuff_head_cache, skb);
break;
case SKB_FCLONE_ORIG:
fclone_ref = (atomic_t *) (skb + 2);
+ skb_check_and_set_free(skb, SKB_STATE_ALLOCATED_MASK);
+
if (atomic_dec_and_test(fclone_ref))
kmem_cache_free(skbuff_fclone_cache, skb);
break;
@@ -374,6 +397,7 @@ static void kfree_skbmem(struct sk_buff *skb)
* fast-cloning again.
*/
skb->fclone = SKB_FCLONE_UNAVAILABLE;
+ skb_check_and_set_free(skb, SKB_STATE_ALLOCATED_MASK);
if (atomic_dec_and_test(fclone_ref))
kmem_cache_free(skbuff_fclone_cache, other);
@@ -639,6 +663,7 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
n->fclone = SKB_FCLONE_UNAVAILABLE;
}
+ skb_check_and_set_alloc(n, SKB_STATE_FREE_MASK);
return __skb_clone(n, skb);
}
EXPORT_SYMBOL(skb_clone);
@@ -1248,6 +1273,7 @@ unsigned char *__pskb_pull_tail(struct sk_buff *skb, int delta)
/* Free pulled out fragments. */
while ((list = skb_shinfo(skb)->frag_list) != insp) {
skb_shinfo(skb)->frag_list = list->next;
+ skb_check_and_set_queued(list, SKB_STATE_QUEUED_MASK);
kfree_skb(list);
}
/* And insert new clone at head. */
@@ -2646,6 +2672,7 @@ skip_fraglist:
err:
while ((skb = segs)) {
segs = skb->next;
+ skb_check_and_set_queued(skb, SKB_STATE_QUEUED_MASK);
kfree_skb(skb);
}
return ERR_PTR(err);
@@ -2760,19 +2787,69 @@ done:
}
EXPORT_SYMBOL_GPL(skb_gro_receive);
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+/*
+ * skb_add_callsite_id_set - Register the sk_buff callsite ID set information
+ */
+static void skb_add_callsite_id_set(void)
+{
+ callsite_set_register(&skb_callsite_set);
+}
+
+/*
+ * Initialize an sk_buff for state validation
+ * @skb: Pointer to the sk_buff to initialize
+ */
+static void skb_kmem_cache_ctor(struct sk_buff *skb)
+{
+ skb->state = SKB_STATE_FREE;
+ skb->last_op = CALLSITE_UNSET;
+}
+
+/*
+ * Initialize an sk_buff from skb_head_cache for state validation
+ * @c: Pointer to the associated kmem_cache structure
+ * @p: Pointer to the particular object in the cache to be initialized
+ */
+static void skb_head_cache_ctor(void *p)
+{
+ skb_kmem_cache_ctor((struct sk_buff *) p);
+}
+
+/*
+ * Initialize an sk_buff from skb_clone_cache for state validation
+ * @c: Pointer to the associated kmem_cache structure
+ * @p: Pointer to the particular object in the cache to be initialized
+ */
+static void skb_fclone_cache_ctor(void *p)
+{
+ struct sk_buff *skb = p;
+ skb_kmem_cache_ctor(skb);
+ skb_kmem_cache_ctor(skb + 1);
+}
+#else
+static void skb_add_callsite_id_set(void)
+{
+}
+
+#define skb_head_cache_ctor NULL
+#define skb_fclone_cache_ctor NULL
+#endif
+
void __init skb_init(void)
{
skbuff_head_cache = kmem_cache_create("skbuff_head_cache",
sizeof(struct sk_buff),
0,
SLAB_HWCACHE_ALIGN|SLAB_PANIC,
- NULL);
+ skb_head_cache_ctor);
skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
(2*sizeof(struct sk_buff)) +
sizeof(atomic_t),
0,
SLAB_HWCACHE_ALIGN|SLAB_PANIC,
- NULL);
+ skb_fclone_cache_ctor);
+ skb_add_callsite_id_set();
}
/**
@@ -3069,3 +3146,108 @@ void __skb_warn_lro_forwarding(const struct sk_buff *skb)
" while LRO is enabled\n", skb->dev->name);
}
EXPORT_SYMBOL(__skb_warn_lro_forwarding);
+
+#ifdef CONFIG_DEBUG_SKB_STATE_CHANGES
+
+struct callsite_set skb_callsite_set = CALLSITE_SET_INIT("skb_callset_set",
+ skb_callsite_set, CONFIG_DEBUG_SKB_ID_WIDTH);
+EXPORT_SYMBOL(skb_callsite_set);
+
+static const char *skb_check_names[] = {
+ [SKB_STATE_INVALID] = "Invalid",
+ [SKB_STATE_FREE] = "Free",
+ [SKB_STATE_ALLOCATED] = "Allocated",
+ [SKB_STATE_QUEUED] = "Queued",
+};
+
+/*
+* Returns a string corresponding to the current sk_buff check state.
+* @state: State whose name is to be determined
+* Returns a pointer to the corresponding string. If the state isn't
+* valid, it will return a pointer to a string indicating an invalid
+* state.
+*/
+static const char *skb_check_name(enum skb_check_state state)
+{
+ const char *result;
+
+ if (state >= ARRAY_SIZE(skb_check_names))
+ result = skb_check_names[SKB_STATE_INVALID];
+
+ else {
+ result = skb_check_names[state];
+
+ if (!result)
+ result = skb_check_names[SKB_STATE_INVALID];
+ }
+
+ return result;
+}
+
+/*
+* Reports an invalid state
+* @skb: Pointer to the &struct sk_buff with an invalid state
+* @expected: Mask of valid states
+*/
+void skb_report_invalid(const struct sk_buff *skb, unsigned expected)
+{
+#define INDENT " "
+ unsigned nbytes;
+ enum skb_check_state actual = skb->state;
+ enum skb_check_state i;
+
+ pr_err("Invalid sk_buff detected at 0x%p state %d (%s)\n",
+ skb, actual, skb_check_name(actual));
+
+ pr_err("Valid states:");
+ for (i = 0; i < SKB_STATE_NUM; i++) {
+ if (skb_check2mask(i) & expected)
+ pr_cont(" %s", skb_check_name(i));
+ }
+ pr_cont("\n");
+
+ /* Print the last allocation and free. The specific address is that
+ * of the call to the inline *_trace function and so could be slightly
+ * different than the call to the *_log function, but it will
+ * be very close. */
+ pr_err(INDENT "last operation: ");
+ callsite_print_where_by_id(&skb_callsite_set, skb->last_op);
+
+ /* If we seem to be in reasonable shape, try to dump a bit of the
+ * buffer so that we have more debugging information. Note that we
+ * only try this if the state of the buffer indicates that we should
+ * be able to trust the sk_buff buffer pointers */
+ switch (skb->state) {
+ case SKB_STATE_ALLOCATED:
+ case SKB_STATE_QUEUED:
+ /* Only try to do the dump if we don't have fragments because
+ * the system might be in too bad a shape to do the mapping
+ * into the kernel virtual address space. */
+ if (skb_is_nonlinear(skb))
+ pr_err("sk_buff is non-linear; skipping print\n");
+
+ else {
+ nbytes = min(96u, (size_t) (skb->end - skb->head));
+ pr_err("sk_buff contents (data offset from head: 0x%x "
+ "bytes)\n", skb->data - skb->head);
+ print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1,
+ skb->head, nbytes, 0);
+ }
+ break;
+ default:
+ pr_err("sk_buff is neither allocated nor queued; "
+ "skipping print\n");
+ break;
+ }
+
+ /*
+ * There is a good chance that the system is compromised when this is
+ * called, but it is possible the problem is limited to a single
+ * driver and may not recur very frequently. Since we are dealing with
+ * networking code, minor errors may be corrected, so BUG() instead
+ * of panicing.
+ */
+ BUG();
+}
+EXPORT_SYMBOL(skb_report_invalid);
+#endif
^ permalink raw reply related
* Re: [PATCH] netconsole: queue console messages to send later
From: Flavio Leitner @ 2010-06-08 0:37 UTC (permalink / raw)
To: David Miller
Cc: netdev, amwang, fubar, mpm, gospo, nhorman, jmoyer, shemminger,
linux-kernel, bridge, bonding-devel
In-Reply-To: <20100607.165024.135517125.davem@davemloft.net>
On Mon, Jun 07, 2010 at 04:50:24PM -0700, David Miller wrote:
> From: Flavio Leitner <fleitner@redhat.com>
> Date: Mon, 7 Jun 2010 16:24:52 -0300
>
> > There are some networking drivers that hold a lock in the
> > transmit path. Therefore, if a console message is printed
> > after that, netconsole will push it through the transmit path,
> > resulting in a deadlock.
> >
> > This patch fixes the re-injection problem by queuing the console
> > messages in a preallocated circular buffer and then scheduling a
> > workqueue to send them later with another context.
> >
> > Signed-off-by: Flavio Leitner <fleitner@redhat.com>
>
> You absolutely and positively MUST NOT do this. Otherwise netconsole
> becomes completely useless. Your idea has been proposed several times
> as far back as 6 years ago, it was unacceptable then and it's
> unacceptable now.
>
> The whole point of netconsole is that we may be deep in an interrupt
> or other atomic context, the machine is about to hard hang, and it's
> absolutely essential that we get out any and all kernel logging
> messages that we can, immediately.
Got it. I've never assumed that netconsole would work reliable on
such situations, so I thought as we have better ways now it would
be helpful. See another idea below.
> There may not be another timer or workqueue able to execute after the
> printk() we're trying to emit. We may never get to that point.
What if in the netpoll, before we push the skb to the driver, we check
for a bit saying that it's already pushing another skb. In this case,
queue the new skb inside of netpoll and soon as the first call returns
and try to clear the bit, it will send the next skb?
printk("message 1")
...
netconsole called
netpoll sets the flag bit
pushes to the bonding driver which does another printk("message 2")
netconsole called again
netpoll checks for the flag, queue the message, returns.
so, bonding can finish up to send the first message
netpoll is about to return, checks for new queued messages, and pushes them.
bonding finishes up to send the second message
....
No deadlocks, skbs are ordered and still under the same opportunity
to send something. Does it sound acceptable?
It's off the top of my head, so probably this idea has some problems.
> Fix the locking in the drivers or layers that cause the issue instead
> of breaking netconsole.
Someday, somewhere, I know because I did this before, someone will
use a debugging printk() and will see the entire box hanging with
absolutely no message in any console because of this problem.
I'm not saying that fixing driver isn't the right way to go but
it seems not enough to me.
--
Flavio
^ permalink raw reply
* Re: [PATCH net-2.6] pkt_sched: gen_estimator: add a new lock
From: Changli Gao @ 2010-06-08 1:00 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Stephen Hemminger, Jarek Poplawski,
Patrick McHardy
In-Reply-To: <1275931108.2545.168.camel@edumazet-laptop>
On Tue, Jun 8, 2010 at 1:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> [PATCH net-2.6] pkt_sched: gen_estimator: add a new lock
>
> gen_kill_estimator() / gen_new_estimator() is not always called with
> RTNL held.
>
> net/netfilter/xt_RATEEST.c is one user of these API that do not hold
> RTNL, so random corruptions can occur between "tc" and "iptables"
>
> Add a new fine grained lock instead of trying to use RTNL in xt_RATEEST
>
Why not use RTNL in xt_RATEEST? It seems xt_RATEEST misuse the APIs.
and I think gen_replace_estimator is expected to be an atomic operation.
And gen_estimator_active() is also assumed to be called with RTNL locked.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [Patch] infiniband: check local reserved ports
From: Cong Wang @ 2010-06-08 2:23 UTC (permalink / raw)
To: Roland Dreier
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, Tetsuo Handa,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
sean.hefty-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <adad3w2onhs.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org>
On 06/07/10 23:45, Roland Dreier wrote:
> > So this patch looks good for you? :)
>
> Yes, will queue it up, thanks.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [linux-pm] [RFC] pm_qos: get rid of the allocation in pm_qos_add_request()
From: mark gross @ 2010-06-08 3:40 UTC (permalink / raw)
To: James Bottomley
Cc: markgross, Takashi Iwai, netdev, pm list, alsa-devel,
Jaroslav Kysela
In-Reply-To: <1275879019.7227.564.camel@mulgrave.site>
On Sun, Jun 06, 2010 at 10:50:19PM -0400, James Bottomley wrote:
> On Sun, 2010-06-06 at 16:10 -0700, mark gross wrote:
> > On Sat, Jun 05, 2010 at 02:20:14PM -0500, James Bottomley wrote:
> > > [alsa-devel says it's a moderated list, so feel free to drop before
> > > replying]
> > >
> > > Since every caller has to squirrel away the returned pointer anyway,
> > > they might as well supply the memory area. This fixes a bug in a few of
> > > the call sites where the returned pointer was dereferenced without
> > > checking it for NULL (which gets returned if the kzalloc failed).
> > >
> > > I'd like to hear how sound and netdev feels about this: it will add
> > > about two more pointers worth of data to struct netdev and struct
> > > snd_pcm_substream .. but I think it's worth it. If you're OK, I'll add
> > > your acks and send through the pm tree.
> > >
> > > This also looks to me like an android independent clean up (even though
> > > it renders the request_add atomically callable). I also added include
> > > guards to include/linux/pm_qos_params.h
> > >
> > > James
> > >
> > > ---
> > >
> > > drivers/net/e1000e/netdev.c | 17 ++++------
> > > drivers/net/igbvf/netdev.c | 9 ++---
> > > drivers/net/wireless/ipw2x00/ipw2100.c | 12 +++---
> > > include/linux/netdevice.h | 2 +-
> > > include/linux/pm_qos_params.h | 12 +++++--
> > > include/sound/pcm.h | 2 +-
> > > kernel/pm_qos_params.c | 55 ++++++++++++++++---------------
> > > sound/core/pcm_native.c | 12 ++-----
> > > 8 files changed, 60 insertions(+), 61 deletions(-)
> > >
> > > diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
> > > index 24507f3..47ea62f 100644
> > > --- a/drivers/net/e1000e/netdev.c
> > > +++ b/drivers/net/e1000e/netdev.c
> > > @@ -2901,10 +2901,10 @@ static void e1000_configure_rx(struct e1000_adapter *adapter)
> > > * dropped transactions.
> > > */
> > > pm_qos_update_request(
> > > - adapter->netdev->pm_qos_req, 55);
> > > + &adapter->netdev->pm_qos_req, 55);
> > > } else {
> > > pm_qos_update_request(
> > > - adapter->netdev->pm_qos_req,
> > > + &adapter->netdev->pm_qos_req,
> > > PM_QOS_DEFAULT_VALUE);
> > > }
> > > }
> > > @@ -3196,9 +3196,9 @@ int e1000e_up(struct e1000_adapter *adapter)
> > >
> > > /* DMA latency requirement to workaround early-receive/jumbo issue */
> > > if (adapter->flags & FLAG_HAS_ERT)
> > > - adapter->netdev->pm_qos_req =
> > > - pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
> > > - PM_QOS_DEFAULT_VALUE);
> > > + pm_qos_add_request(&adapter->netdev->pm_qos_req,
> > > + PM_QOS_CPU_DMA_LATENCY,
> > > + PM_QOS_DEFAULT_VALUE);
> > >
> > > /* hardware has been reset, we need to reload some things */
> > > e1000_configure(adapter);
> > > @@ -3263,11 +3263,8 @@ void e1000e_down(struct e1000_adapter *adapter)
> > > e1000_clean_tx_ring(adapter);
> > > e1000_clean_rx_ring(adapter);
> > >
> > > - if (adapter->flags & FLAG_HAS_ERT) {
> > > - pm_qos_remove_request(
> > > - adapter->netdev->pm_qos_req);
> > > - adapter->netdev->pm_qos_req = NULL;
> > > - }
> > > + if (adapter->flags & FLAG_HAS_ERT)
> > > + pm_qos_remove_request(&adapter->netdev->pm_qos_req);
> > >
> > > /*
> > > * TODO: for power management, we could drop the link and
> > > diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
> > > index 5e2b2a8..5da569f 100644
> > > --- a/drivers/net/igbvf/netdev.c
> > > +++ b/drivers/net/igbvf/netdev.c
> > > @@ -48,7 +48,7 @@
> > > #define DRV_VERSION "1.0.0-k0"
> > > char igbvf_driver_name[] = "igbvf";
> > > const char igbvf_driver_version[] = DRV_VERSION;
> > > -struct pm_qos_request_list *igbvf_driver_pm_qos_req;
> > > +struct pm_qos_request_list igbvf_driver_pm_qos_req;
> > > static const char igbvf_driver_string[] =
> > > "Intel(R) Virtual Function Network Driver";
> > > static const char igbvf_copyright[] = "Copyright (c) 2009 Intel Corporation.";
> > > @@ -2902,8 +2902,8 @@ static int __init igbvf_init_module(void)
> > > printk(KERN_INFO "%s\n", igbvf_copyright);
> > >
> > > ret = pci_register_driver(&igbvf_driver);
> > > - igbvf_driver_pm_qos_req = pm_qos_add_request(PM_QOS_CPU_DMA_LATENCY,
> > > - PM_QOS_DEFAULT_VALUE);
> > > + pm_qos_add_request(igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
> > should be:
> >
> > + pm_qos_add_request(&igbvf_driver_pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
>
> Yes, thanks ... must be not compiling this driver for some reason in the
> test case.
>
I think we need to do better with the file gard or fix e100e/netdev.c to
avoid panicking when un-loading the e1000e driver.
I'm on the fence whether to address the e1000e brain damage, or put in
protection from whatever is causing the crash. (which funny enough I
can't get a serial console to that crashing system to share it with
you. but I'm pretty sure an un-initialized netdev->pm_qos_request is
getting passed in (with class==0) resulting in the de-referencing of
NULL in the update_target function.
I'll be other abusers are doing something similar we'll need to guard
again. Would you like me to send an updated patch with some of this
guarding in place?
--mgross
^ permalink raw reply
* Re: 2.6.35-rc2-git1 - include/linux/cgroup.h:534 invoked rcu_dereference_check() without protection!
From: Miles Lane @ 2010-06-08 4:16 UTC (permalink / raw)
To: paulmck
Cc: Vivek Goyal, Eric Paris, Lai Jiangshan, Ingo Molnar,
Peter Zijlstra, LKML, nauman, eric.dumazet, netdev, Jens Axboe,
Gui Jianfeng, Li Zefan, Johannes Berg
In-Reply-To: <20100608001929.GF2387@linux.vnet.ibm.com>
On Mon, Jun 7, 2010 at 8:19 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Mon, Jun 07, 2010 at 02:14:25PM -0400, Miles Lane wrote:
>> Hi All,
>>
>> I just reproduced a warning I reported quite a while ago. Is a patch
>> for this in the pipeline?
>
> I proposed a patch, thinking that it was a false positive. Peter Zijlstra
> pointed out that there was a real race, and proposed an alternative patch,
> which may be found at http://lkml.org/lkml/2010/4/22/603.
>
> Could you please test Peter's patch and let us know if it cures the problem?
>
> Thanx, Paul
>
>> [ 0.167267] [ INFO: suspicious rcu_dereference_check() usage. ]
>> [ 0.167396] ---------------------------------------------------
>> [ 0.167526] include/linux/cgroup.h:534 invoked
>> rcu_dereference_check() without protection!
>> [ 0.167728]
>> [ 0.167729] other info that might help us debug this:
>> [ 0.167731]
>> [ 0.168092]
>> [ 0.168093] rcu_scheduler_active = 1, debug_locks = 1
>> [ 0.168337] no locks held by watchdog/0/5.
>> [ 0.168462]
>> [ 0.168463] stack backtrace:
>> [ 0.168704] Pid: 5, comm: watchdog/0 Not tainted 2.6.35-rc2-git1 #8
>> [ 0.168834] Call Trace:
>> [ 0.168965] [<ffffffff81064e9c>] lockdep_rcu_dereference+0x9d/0xa5
>> [ 0.169100] [<ffffffff8102c1ce>] task_subsys_state+0x59/0x70
>> [ 0.169232] [<ffffffff8103189b>] __sched_setscheduler+0x19d/0x2f8
>> [ 0.169365] [<ffffffff8102a5ef>] ? need_resched+0x1e/0x28
>> [ 0.169497] [<ffffffff813c7d01>] ? schedule+0x586/0x619
>> [ 0.169628] [<ffffffff81081c33>] ? watchdog+0x0/0x8c
>> [ 0.169758] [<ffffffff81031a11>] sched_setscheduler+0xe/0x10
>> [ 0.169889] [<ffffffff81081c5d>] watchdog+0x2a/0x8c
>> [ 0.170010] [<ffffffff81081c33>] ? watchdog+0x0/0x8c
>> [ 0.170141] [<ffffffff81054a82>] kthread+0x89/0x91
>> [ 0.170274] [<ffffffff81003054>] kernel_thread_helper+0x4/0x10
>> [ 0.170405] [<ffffffff813ca480>] ? restore_args+0x0/0x30
>> [ 0.170536] [<ffffffff810549f9>] ? kthread+0x0/0x91
>> [ 0.170667] [<ffffffff81003050>] ? kernel_thread_helper+0x0/0x10
>> [ 0.176751] lockdep: fixing up alternatives.
>
With the patch, I get:
[ 0.151274] [ INFO: suspicious rcu_dereference_check() usage. ]
[ 0.151390] ---------------------------------------------------
[ 0.151520] include/linux/cgroup.h:534 invoked
rcu_dereference_check() without protection!
[ 0.151723]
[ 0.151724] other info that might help us debug this:
[ 0.151726]
[ 0.151999]
[ 0.151999] rcu_scheduler_active = 1, debug_locks = 1
[ 0.151999] 2 locks held by kthreadd/10:
[ 0.151999] #0: (key){......}, at: [<ffffffff81036578>] complete+0x1c/0x4e
[ 0.151999] #1: (&rq->lock){-.-...}, at: [<ffffffff81037875>]
select_task_rq_fair+0x21f/0x791
[ 0.151999]
[ 0.151999] stack backtrace:
[ 0.151999] Pid: 10, comm: kthreadd Not tainted 2.6.35-rc2-git1 #11
[ 0.151999] Call Trace:
[ 0.151999] [<ffffffff81070a45>] lockdep_rcu_dereference+0x9d/0xa5
[ 0.151999] [<ffffffff8103675e>] task_subsys_state+0x59/0x70
[ 0.151999] [<ffffffff8103799a>] select_task_rq_fair+0x344/0x791
[ 0.151999] [<ffffffff81037335>] ? task_rq_lock+0x68/0x9d
[ 0.151999] [<ffffffff811d62f3>] ? do_raw_spin_lock+0x79/0x13e
[ 0.151999] [<ffffffff81037335>] ? task_rq_lock+0x68/0x9d
[ 0.151999] [<ffffffff8103ac1e>] select_task_rq+0x13/0x44
[ 0.151999] [<ffffffff810417c3>] try_to_wake_up+0xf2/0x37d
[ 0.151999] [<ffffffff81041a5b>] default_wake_function+0xd/0xf
[ 0.151999] [<ffffffff81034272>] __wake_up_common+0x49/0x7f
[ 0.151999] [<ffffffff81036596>] complete+0x3a/0x4e
[ 0.151999] [<ffffffff8105b598>] ? worker_thread+0x0/0x3a7
[ 0.151999] [<ffffffff8105f7d0>] kthread+0x73/0x91
[ 0.151999] [<ffffffff8100aba4>] kernel_thread_helper+0x4/0x10
[ 0.151999] [<ffffffff813e3694>] ? restore_args+0x0/0x30
[ 0.151999] [<ffffffff8105f75d>] ? kthread+0x0/0x91
[ 0.151999] [<ffffffff8100aba0>] ? kernel_thread_helper+0x0/0x10
^ permalink raw reply
* Re: [PATCHv2 1/2] net: Enable 64-bit net device statistics on 32-bit architectures
From: David Miller @ 2010-06-08 4:18 UTC (permalink / raw)
To: bhutchings; +Cc: shemminger, arnd, netdev, linux-net-drivers
In-Reply-To: <1275689093.2095.36.camel@achroite.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 04 Jun 2010 23:04:53 +0100
> +#if BITS_PER_LONG == 64
> +#define NET_DEVICE_STATS_DEFINE(name) u64 name
> +#elif defined(__LITTLE_ENDIAN)
> +#define NET_DEVICE_STATS_DEFINE(name) u32 name, pad_ ## name
> +#else
> +#define NET_DEVICE_STATS_DEFINE(name) u32 pad_ ## name, name
> +#endif
> +
...
> + NET_DEVICE_STATS_DEFINE(rx_packets);
> + NET_DEVICE_STATS_DEFINE(tx_packets);
> + NET_DEVICE_STATS_DEFINE(rx_bytes);
...
> static const char fmt[] = "%30s %12lu\n";
> + static const char fmt64[] = "%30s %12llu\n";
...
> + seq_printf(seq, fmt64, "total frames received", stats->rx_packets);
> + seq_printf(seq, fmt64, "total bytes received", stats->rx_bytes);
> + seq_printf(seq, fmt64, "Broadcast/Multicast Rcvd", stats->multicast);
I guess you only built this on a 64-bit platform that defines u64 as a
long long type.
The rest of the world will receive tons of warnings for these print
statements.
You have basically 3 cases to handle:
1) u64 defined as "unsigned long long"
2) u64 defined as "unsigned long"
3) u32 defined as "unsigned int"
And the whole tree needs to be inspected to make sure there isn't going
to be fallout in areas your patch didn't take care of wrt. printf format
strings and the like.
What was always "unsigned long" is now a variable type, therefore using
a fixed printf format string is impossible unless you always cast these
things when passed in as printf arguments.
^ permalink raw reply
* Re: [PATCH v3] netdev:bfin_mac: reclaim and free tx skb as soon as possible after transfer
From: David Miller @ 2010-06-08 4:22 UTC (permalink / raw)
To: sonic.adi; +Cc: netdev, uclinux-dist-devel
In-Reply-To: <1275907104.9082.1.camel@eight.analog.com>
From: sonic zhang <sonic.adi@gmail.com>
Date: Mon, 7 Jun 2010 18:38:24 +0800
>
> + if (timer_pending(&lp->tx_reclaim_timer))
> + del_timer(&(lp->tx_reclaim_timer));
> +
Please remove the excess parenthesis around lp->tx_reclaim_timer being
passed to del_timer().
Also, you can unconditionally call del_timer(). If the timer isn't running
the call won't do anything.
But you have to do something to make sure you don't race with the code that
enables the timer, f.e. what keeps the timer from being scheduled right
after you make this del_timer() call?
^ permalink raw reply
* Re: [PATCH net-next-2.6] macvlan: use call_rcu for port free
From: David Miller @ 2010-06-08 4:26 UTC (permalink / raw)
To: eric.dumazet; +Cc: jpirko, netdev, kaber
In-Reply-To: <1275912936.2545.48.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Jun 2010 14:15:36 +0200
> Le lundi 07 juin 2010 à 13:36 +0200, Jiri Pirko a écrit :
>> Use call_rcu rather than synchronize_rcu.
>>
>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>> ---
>> drivers/net/macvlan.c | 12 ++++++++++--
>> 1 files changed, 10 insertions(+), 2 deletions(-)
>>
>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ip: Router Alert RCU conversion
From: David Miller @ 2010-06-08 4:26 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1275916328.2545.64.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Jun 2010 15:12:08 +0200
> Straightforward conversion to RCU.
>
> One rwlock becomes a spinlock, and is static.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] igmp: avoid two atomic ops in igmp_rcv()
From: David Miller @ 2010-06-08 4:26 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1275916630.2545.68.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Jun 2010 15:17:10 +0200
> in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv4: avoid two atomic ops in ip_rt_redirect()
From: David Miller @ 2010-06-08 4:26 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1275916983.2545.73.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Jun 2010 15:23:03 +0200
> in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] net: avoid two atomic ops in ip_rcv_options()
From: David Miller @ 2010-06-08 4:26 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1275918886.2545.87.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Jun 2010 15:54:46 +0200
> in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: 2.6.35-rc2-git1 - lib/idr.c:605 invoked rcu_dereference_check() without protection!
From: Miles Lane @ 2010-06-08 4:28 UTC (permalink / raw)
To: paulmck
Cc: Vivek Goyal, Eric Paris, David Woodhouse, Lai Jiangshan,
Ingo Molnar, Peter Zijlstra, LKML, nauman, eric.dumazet, netdev,
Jens Axboe, Gui Jianfeng, Li Zefan, Johannes Berg
In-Reply-To: <20100608001234.GE2387@linux.vnet.ibm.com>
On Mon, Jun 7, 2010 at 8:12 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Mon, Jun 07, 2010 at 02:23:17PM -0400, Miles Lane wrote:
>> [ 2.677955] [ INFO: suspicious rcu_dereference_check() usage. ]
>> [ 2.679089] ---------------------------------------------------
>> [ 2.680276] lib/idr.c:605 invoked rcu_dereference_check() without protection!
>> [ 2.681499]
>> [ 2.681500] other info that might help us debug this:
>> [ 2.681501]
>> [ 2.685509]
>> [ 2.685510] rcu_scheduler_active = 1, debug_locks = 1
>> [ 2.688221] 1 lock held by swapper/1:
>> [ 2.689587] #0: (mtd_table_mutex){+.+...}, at:
>> [<ffffffff812bea45>] register_mtd_user+0x1a/0x69
>> [ 2.691096]
>> [ 2.691098] stack backtrace:
>> [ 2.694059] Pid: 1, comm: swapper Not tainted 2.6.35-rc2-git1 #8
>> [ 2.695601] Call Trace:
>> [ 2.697243] [<ffffffff81064e9c>] lockdep_rcu_dereference+0x9d/0xa5
>> [ 2.698868] [<ffffffff811b9c86>] idr_get_next+0x60/0x124
>> [ 2.700556] [<ffffffff812be779>] __mtd_next_device+0x1b/0x1d
>> [ 2.702238] [<ffffffff812bea7c>] register_mtd_user+0x51/0x69
>> [ 2.703964] [<ffffffff816cca45>] init_mtdchar+0xb3/0xd3
>> [ 2.705686] [<ffffffff816cc992>] ? init_mtdchar+0x0/0xd3
>> [ 2.707470] [<ffffffff810001ef>] do_one_initcall+0x59/0x14e
>> [ 2.709255] [<ffffffff816a768a>] kernel_init+0x144/0x1ce
>> [ 2.711082] [<ffffffff81003054>] kernel_thread_helper+0x4/0x10
>> [ 2.712862] [<ffffffff813ca480>] ? restore_args+0x0/0x30
>> [ 2.714647] [<ffffffff816a7546>] ? kernel_init+0x0/0x1ce
>> [ 2.716415] [<ffffffff81003050>] ? kernel_thread_helper+0x0/0x10
>
> This looks like a new one! Does the following patch take care of it?
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit 2d54a6c31b72c902b09d365e9c66205a5c07e549
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Date: Mon Jun 7 17:09:45 2010 -0700
>
> idr: fix RCU lockdep splat in idr_get_next()
>
> Convert to rcu_dereference_raw() given that many callers may have many
> different locking models.
>
> Located-by: Miles Lane <miles.lane@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> diff --git a/lib/idr.c b/lib/idr.c
> index 2eb1dca..f099f25 100644
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -599,7 +599,7 @@ void *idr_get_next(struct idr *idp, int *nextidp)
> /* find first ent */
> n = idp->layers * IDR_BITS;
> max = 1 << n;
> - p = rcu_dereference(idp->top);
> + p = rcu_dereference_raw(idp->top);
> if (!p)
> return NULL;
>
> @@ -607,7 +607,7 @@ void *idr_get_next(struct idr *idp, int *nextidp)
> while (n > 0 && p) {
> n -= IDR_BITS;
> *paa++ = p;
> - p = rcu_dereference(p->ary[(id >> n) & IDR_MASK]);
> + p = rcu_dereference_raw(p->ary[(id >> n) & IDR_MASK]);
> }
>
> if (p) {
>
Tested. Looks good!
^ permalink raw reply
* Re: [PATCH net-2.6] pkt_sched: gen_estimator: add a new lock
From: Eric Dumazet @ 2010-06-08 4:30 UTC (permalink / raw)
To: Changli Gao
Cc: David Miller, netdev, Stephen Hemminger, Jarek Poplawski,
Patrick McHardy
In-Reply-To: <AANLkTik7k__McJk8VCu0WP1RYEDHUfBJA4dyGYQTUmBV@mail.gmail.com>
Le mardi 08 juin 2010 à 09:00 +0800, Changli Gao a écrit :
> On Tue, Jun 8, 2010 at 1:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> > [PATCH net-2.6] pkt_sched: gen_estimator: add a new lock
> >
> > gen_kill_estimator() / gen_new_estimator() is not always called with
> > RTNL held.
> >
> > net/netfilter/xt_RATEEST.c is one user of these API that do not hold
> > RTNL, so random corruptions can occur between "tc" and "iptables"
> >
> > Add a new fine grained lock instead of trying to use RTNL in xt_RATEEST
> >
>
> Why not use RTNL in xt_RATEEST? It seems xt_RATEEST misuse the APIs.
>
> and I think gen_replace_estimator is expected to be an atomic operation.
>
> And gen_estimator_active() is also assumed to be called with RTNL locked.
>
Thank you for asking this question.
Because I want to kill RTNL when possible, I dont even want to try
adding RTNL to xt_RATEEST and solve all lock dependencies it might
raise.
RTNL = Big and Horrible Network LOCK
You never got blocked because of this RTNL thing, dont you ?
I did. And it sucks, because when you hit this, you are in a hurry and
locating the bottleneck takes lot of time.
RTNL is the thing we must hold during device register / unregister.
Its locked for long delays because of all synchronize_rcu() that must be
done, and that is already a big problem on some setups.
Every time someone adds a RTNL requirement, you can be sure another guy
will zap it during following ten years.
Let's do this right now, not later.
For an example of horrible rtnl behavior, take a look at following
construct :
if (!rtnl_trylock())
return restart_syscall();
I saw hundred of udev looping, trying to get rtnl to dump some
information. (Patrick added a rtnl requirement to all dump operations,
and it sucks)
^ permalink raw reply
* Re: [PATCH][RFC] Check sk_buff states
From: Mitchell Erblich @ 2010-06-08 4:46 UTC (permalink / raw)
To: David VomLehn; +Cc: netdev
In-Reply-To: <20100608003055.GA29405@dvomlehn-lnx2.corp.sa.net>
On Jun 7, 2010, at 5:30 PM, David VomLehn wrote:
> This uses the oobparam and callsite infrastructure to implement sk_buff
> state checking and error reporting. Possible states of an sk_buff are:
> SKB_STATE_FREE - Is not currently in use
> SKB_STATE_ALLOCATED - Has been allocated, but is not on a queue
> SKB_STATE_QUEUED - Is allocated and on a queue
> Since there are only three states, two bits suffice to record the state of
> an sk_buff structure, so checking for consistent state is easy. (For you
> weenies, the fourth possible state *is* flagged as an error).
... cut...
Just my two cents,
Just initially thinking of the state names & using only two bits
If a 3rd bit, then it could indicate exclusive or shared (double free then acceptable if shared)
kmem_alloc, malloc, etc actually allocate structs
when something is allocated but not associated with anything, then it has no reference
so then why not?
xxx_STATE_INVALID: both bits unset
xxx_STATE_ALLOCATED: bit 0 set and bit 1 not set
xxx_STATE_REFERENCED : could be exclusive or shared, bit 0 unset & bit 1 set
xxx_STATE_QUEUED: both bits set
Mitchell Erblich
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv4: avoid two atomic ops in ip_rt_redirect()
From: David Miller @ 2010-06-08 4:51 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <20100607.212622.267982352.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Mon, 07 Jun 2010 21:26:22 -0700 (PDT)
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 07 Jun 2010 15:23:03 +0200
>
>> in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> Applied.
I needed to modify this patch to get it to build.
You left a code label with no code afterwards at the end of
ip_rt_redirect() when CONFIG_IP_ROUTE_VERBOSE is disabled,
which breaks the build.
I fixed it by adding a semicolon.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv4: avoid two atomic ops in ip_rt_redirect()
From: Eric Dumazet @ 2010-06-08 4:56 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20100607.215119.176661892.davem@davemloft.net>
Le lundi 07 juin 2010 à 21:51 -0700, David Miller a écrit :
> From: David Miller <davem@davemloft.net>
> Date: Mon, 07 Jun 2010 21:26:22 -0700 (PDT)
>
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 07 Jun 2010 15:23:03 +0200
> >
> >> in_dev_get() -> __in_dev_get_rcu() in a rcu protected function.
> >>
> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >
> > Applied.
>
> I needed to modify this patch to get it to build.
>
> You left a code label with no code afterwards at the end of
> ip_rt_redirect() when CONFIG_IP_ROUTE_VERBOSE is disabled,
> which breaks the build.
>
> I fixed it by adding a semicolon.
OK, thanks David, I missed this.
^ permalink raw reply
* Re: [PATCH net-2.6] pkt_sched: gen_estimator: add a new lock
From: Changli Gao @ 2010-06-08 4:57 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Stephen Hemminger, Jarek Poplawski,
Patrick McHardy
In-Reply-To: <1275971457.2775.40.camel@edumazet-laptop>
On Tue, Jun 8, 2010 at 12:30 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Thank you for asking this question.
>
> Because I want to kill RTNL when possible, I dont even want to try
> adding RTNL to xt_RATEEST and solve all lock dependencies it might
> raise.
>
> RTNL = Big and Horrible Network LOCK
>
It is much like the BKL, and should be killed too. Thanks. :)
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH net-2.6] pkt_sched: gen_estimator: add a new lock
From: Eric Dumazet @ 2010-06-08 4:58 UTC (permalink / raw)
To: Changli Gao
Cc: David Miller, netdev, Stephen Hemminger, Jarek Poplawski,
Patrick McHardy
In-Reply-To: <AANLkTik7k__McJk8VCu0WP1RYEDHUfBJA4dyGYQTUmBV@mail.gmail.com>
Le mardi 08 juin 2010 à 09:00 +0800, Changli Gao a écrit :
> and I think gen_replace_estimator is expected to be an atomic operation.
>
> And gen_estimator_active() is also assumed to be called with RTNL locked.
>
My patch fixes a bug of new/kill operators, regardless of RTNL being
held or not. Its should be small enough to be included in linux-2.6.35.
If what you say is right, all gen_replace_estimator() /
gen_estimator_active() callers should still holds RTNL.
I didnt change this part.
If you believe one caller doesnt hold RTNL, please submit another patch.
Then, in net-next-2.6, we can probably cleanup this to remove RTNL
requirement if possible for gen_replace_estimator() /
gen_estimator_active()
Yes, it sounds a bit difficult (three patches instead of a single one),
but this is the how things should be done, step by step.
^ permalink raw reply
* cxgb4i_v4.3 submission
From: Rakesh Ranjan @ 2010-06-08 4:59 UTC (permalink / raw)
To: LK-NetDev, LK-SCSIDev, LK-iSCSIDev
Cc: LKML, Karen Xie, David Miller, James Bottomley, Mike Christie,
Anish Bhatt
The following 3 patches add a new iscsi LLD driver cxgb4i to enable iscsi offload
support on Chelsio's new 1G and 10G cards. This is updated version of previous cxgb4i
patch. Please share you commnets after review.
Changes since cxgb4i_v4.2
1. Removed early returns from some functions, which got added for debugging.
[PATCH 1/3] cxgb4i_v4.3 : add build support
[PATCH 2/3] cxgb4i_v4.3 : libcxgbi common library part
[PATCH 3/3] cxgb4i_v4.3 : main driver files
Regards
Rakesh Ranjan
--
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox