[PATCH] igb: Workaround for igb i210 firmware issue.

[Chris J Arges]

Sometimes firmware may not properly initialize I347AT4_PAGE_SELECT causing
the probe of an igb i210 NIC to fail. This patch adds an addition zeroing of
this register during igb_get_phy_id to workaround this issue.

Thanks for Jochen Henneberg for the idea and original patch.

Signed-off-by: Chris J Arges <christopherar...@gmail.com>
---
 drivers/net/ethernet/intel/igb/e1000_phy.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c 
b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 5b54254..93ec2d0 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -77,6 +77,10 @@ s32 igb_get_phy_id(struct e1000_hw *hw)
s32 ret_val = 0;
u16 phy_id;
 
+   /* ensure phy page selection to fix misconfigured i210 */
+   if (hw->mac.type == e1000_i210)
+   phy->ops.write_reg(hw, I347AT4_PAGE_SELECT, 0);
+
ret_val = phy->ops.read_reg(hw, PHY_ID1, _id);
if (ret_val)
goto out;
-- 
2.7.4

Re: [PATCH 00/33] Compile-time stack metadata validation

[Chris J Arges]

On Fri, Jan 22, 2016 at 01:14:47PM -0600, Josh Poimboeuf wrote:
> On Fri, Jan 22, 2016 at 11:43:48AM -0600, Chris J Arges wrote:
> > On Thu, Jan 21, 2016 at 04:49:04PM -0600, Josh Poimboeuf wrote:
> > > This is v16 of the compile-time stack metadata validation patch set,
> > > along with proposed fixes for most of the warnings it found.  It's based
> > > on the tip/master branch.
> > >
> > Josh,
> > 
> > Looks good, with my config [1] I do still get a few warnings building
> > linux/linux-next.
> > 
> > Here are the warnings:
> > $ grep ^stacktool build.log | grep -v staging
> 
> Thanks for reporting these!
> 
> > stacktool: arch/x86/kvm/vmx.o: vmx_handle_external_intr()+0x67: call 
> > without frame pointer save/setup
> 
> This can be fixed by setting the stack pointer as an output operand for
> the inline asm call in vmx_handle_external_intr().
> 
> Feel free to submit a patch, or I'll get around to it eventually.
> 
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: return 
> > without frame pointer restore
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x89: duplicate 
> > frame pointer save
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x8a: duplicate 
> > frame pointer setup
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: frame 
> > pointer state mismatch
> > stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x0: frame 
> > pointer state mismatch
> 
> These are false positives.  Stacktool is confused by the use of a
> "noreturn" function which it doesn't know about (__reiserfs_panic).
> 
> Unfortunately the only solution I currently have for dealing with global
> noreturn functions is to just hard-code a list of them.  So the short
> term fix would be to add "__reiserfs_panic" to the global_noreturns list
> in tools/stacktool/builtin-check.c.
> 
> I'm still trying to figure out a better way to deal with this type of
> issue, as it's a pain to have to keep a hard-coded list of noreturn
> functions.  Unfortunately that info isn't available in the ELF.
> 

Josh,
Ok I'll hack on the patches above.

> > stacktool: fs/reiserfs/ibalance.o: .text: unexpected end of section
> > stacktool: fs/reiserfs/tail_conversion.o: .text: unexpected end of section
> 
> For some reason I'm not able to recreate these warnings...  Can you
> share one of the .o files?
> 
> -- 
> Josh
> 

Binaries are here:
http://people.canonical.com/~arges/stacktool/

--chris

Re: [PATCH 00/33] Compile-time stack metadata validation

[Chris J Arges]

On Thu, Jan 21, 2016 at 04:49:04PM -0600, Josh Poimboeuf wrote:
> This is v16 of the compile-time stack metadata validation patch set,
> along with proposed fixes for most of the warnings it found.  It's based
> on the tip/master branch.
>
Josh,

Looks good, with my config [1] I do still get a few warnings building
linux/linux-next.

Here are the warnings:
$ grep ^stacktool build.log | grep -v staging
stacktool: arch/x86/kvm/vmx.o: vmx_handle_external_intr()+0x67: call without 
frame pointer save/setup
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: return without 
frame pointer restore
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x89: duplicate frame 
pointer save
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x8a: duplicate frame 
pointer setup
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x9e: frame pointer 
state mismatch
stacktool: fs/reiserfs/namei.o: set_de_name_and_namelen()+0x0: frame pointer 
state mismatch
stacktool: fs/reiserfs/ibalance.o: .text: unexpected end of section
stacktool: fs/reiserfs/tail_conversion.o: .text: unexpected end of section

For vmx_handle_external_intr, I'm wondering if ignoring this function is the
best option.

--

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e2951b6..d19dfb2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "kvm_cache_regs.h"
 #include "x86.h"
 
@@ -8398,6 +8399,7 @@ static void vmx_handle_external_intr(struct kvm_vcpu 
*vcpu)
} else
local_irq_enable();
 }
+STACKTOOL_IGNORE_FUNC(vmx_handle_external_intr);
 
 static bool vmx_has_high_real_mode_segbase(void)
 {

--chris

[1] http://paste.ubuntu.com/14599083/

 
> v15 can be found here:
> 
>   https://lkml.kernel.org/r/cover.1450442274.git.jpoim...@redhat.com
> 
> For more information about the motivation behind this patch set, and
> more details about what it does, see the first patch changelog and
> tools/stacktool/Documentation/stack-validation.txt.
> 
> Patches 1-4 add stacktool and integrate it into the kernel build.
> 
> Patches 5-28 are some proposed fixes for several of the warnings
> reported by stacktool.  They've been compile-tested and boot-tested in a
> VM, but I haven't attempted any meaningful testing for many of them.
> 
> Patches 29-33 add some directories, files, and functions to the
> stacktool whitelist in order to silence false positive warnings.
> 
> v16:
> - fix all allyesconfig warnings, except for staging
> - get rid of STACKTOOL_IGNORE_INSN which is no longer needed
> - remove several whitelists in favor of automatically whitelisting any
>   function with a special instruction like ljmp, lret, or vmrun
> - split up stacktool patch into 3 parts as suggested by Ingo
> - update the global noreturn function list
> - detect noreturn function fallthroughs
> - skip weak functions in noreturn call detection logic
> - add empty function check to noreturn logic
> - allow non-section rela symbols for __ex_table sections
> - support rare switch table case with jmpq *[addr](%rip)
> - don't warn on frame pointer restore without save
> - rearrange patch order a bit
> 
> v15:
> - restructure code for a new cmdline interface "stacktool check" using
>   the new subcommand framework in tools/lib/subcmd
> - fix 32 bit build fail (put __sp at end) in paravirt_types.h patch 10
>   which was reported by 0day
> 
> v14:
> - make tools/include/linux/list.h self-sufficient
> - create FRAME_OFFSET to allow 32-bit code to be able to access function
>   arguments on the stack
> - add FRAME_OFFSET usage in crypto patch 14/24: "Create stack frames in
>   aesni-intel_asm.S"
> - rename "index" -> "idx" to fix build with some compilers
> 
> v13:
> - LDFLAGS order fix from Chris J Arges
> - new warning fix patches from Chris J Arges
> - "--frame-pointer" -> "--check-frame-pointer"
> 
> v12:
> - rename "stackvalidate" -> "stacktool"
> - move from scripts/ to tools/:
>   - makefile rework
>   - make a copy of the x86 insn code (and warn if the code diverges)
>   - use tools/include/linux/list.h
> - move warning macros to a new warn.h file
> - change wording: "stack validation" -> "stack metadata validation"
> 
> v11:
> - attempt to answer the "why" question better in the documentation and
>   commit message
> - s/FP_SAVE/FRAME_BEGIN/ in documentation
> 
> v10:
> - add scripts/mod to directory ignores
> - remove circular dependencies for ignored objects which are built
>   before stackvalidate
> - fix CONFIG_MODVERSIONS incompatibility
> 
> v9:
> - rename FRAME/ENDFRAME -> FRAM

[PATCH] openvswitch: make for_each_node loops work with sparse numa systems

[Chris J Arges]

Some architectures like POWER can have a NUMA node_possible_map that
contains sparse entries. This causes memory corruption with openvswitch
since it allocates flow_cache with a multiple of num_possible_nodes() and
assumes the node variable returned by for_each_node will index into
flow-stats[node].

For example, if node_possible_map is 0x30003, this patch will map node to
node_cnt as follows:
0,1,16,17 = 0,1,2,3

The crash was noticed after 3af229f2 was applied as it changed the
node_possible_map to match node_online_map on boot.
Fixes: 3af229f2071f5b5cb31664be6109561fbe19c861

Signed-off-by: Chris J Arges chris.j.ar...@canonical.com
---
 net/openvswitch/flow.c   | 10 ++
 net/openvswitch/flow_table.c | 18 +++---
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index bc7b0ab..425d45d 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -134,14 +134,14 @@ void ovs_flow_stats_get(const struct sw_flow *flow,
struct ovs_flow_stats *ovs_stats,
unsigned long *used, __be16 *tcp_flags)
 {
-   int node;
+   int node, node_cnt = 0;
 
*used = 0;
*tcp_flags = 0;
memset(ovs_stats, 0, sizeof(*ovs_stats));
 
for_each_node(node) {
-   struct flow_stats *stats = 
rcu_dereference_ovsl(flow-stats[node]);
+   struct flow_stats *stats = 
rcu_dereference_ovsl(flow-stats[node_cnt]);
 
if (stats) {
/* Local CPU may write on non-local stats, so we must
@@ -155,16 +155,17 @@ void ovs_flow_stats_get(const struct sw_flow *flow,
ovs_stats-n_bytes += stats-byte_count;
spin_unlock_bh(stats-lock);
}
+   node_cnt++;
}
 }
 
 /* Called with ovs_mutex. */
 void ovs_flow_stats_clear(struct sw_flow *flow)
 {
-   int node;
+   int node, node_cnt = 0;
 
for_each_node(node) {
-   struct flow_stats *stats = ovsl_dereference(flow-stats[node]);
+   struct flow_stats *stats = 
ovsl_dereference(flow-stats[node_cnt]);
 
if (stats) {
spin_lock_bh(stats-lock);
@@ -174,6 +175,7 @@ void ovs_flow_stats_clear(struct sw_flow *flow)
stats-tcp_flags = 0;
spin_unlock_bh(stats-lock);
}
+   node_cnt++;
}
 }
 
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 4613df8..5d10c54 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -77,7 +77,7 @@ struct sw_flow *ovs_flow_alloc(void)
 {
struct sw_flow *flow;
struct flow_stats *stats;
-   int node;
+   int node, node_cnt = 0;
 
flow = kmem_cache_alloc(flow_cache, GFP_KERNEL);
if (!flow)
@@ -99,9 +99,11 @@ struct sw_flow *ovs_flow_alloc(void)
 
RCU_INIT_POINTER(flow-stats[0], stats);
 
-   for_each_node(node)
+   for_each_node(node) {
if (node != 0)
-   RCU_INIT_POINTER(flow-stats[node], NULL);
+   RCU_INIT_POINTER(flow-stats[node_cnt], NULL);
+   node_cnt++;
+   }
 
return flow;
 err:
@@ -139,15 +141,17 @@ static struct flex_array *alloc_buckets(unsigned int 
n_buckets)
 
 static void flow_free(struct sw_flow *flow)
 {
-   int node;
+   int node, node_cnt = 0;
 
if (ovs_identifier_is_key(flow-id))
kfree(flow-id.unmasked_key);
kfree((struct sw_flow_actions __force *)flow-sf_acts);
-   for_each_node(node)
-   if (flow-stats[node])
+   for_each_node(node) {
+   if (flow-stats[node_cnt])
kmem_cache_free(flow_stats_cache,
-   (struct flow_stats __force 
*)flow-stats[node]);
+   (struct flow_stats __force 
*)flow-stats[node_cnt]);
+   node_cnt++;
+   }
kmem_cache_free(flow_cache, flow);
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] openvswitch: make for_each_node loops work with sparse numa systems

[Chris J Arges]

On Tue, Jul 21, 2015 at 09:24:18AM -0700, Nishanth Aravamudan wrote:
 On 21.07.2015 [10:32:34 -0500], Chris J Arges wrote:
  Some architectures like POWER can have a NUMA node_possible_map that
  contains sparse entries. This causes memory corruption with openvswitch
  since it allocates flow_cache with a multiple of num_possible_nodes() and
 
 Couldn't this also be fixed by just allocationg with a multiple of
 nr_node_ids (which seems to have been the original intent all along)?
 You could then make your stats array be sparse or not.
 

Yea originally this is what I did, but I thought it would be wasting memory.

  assumes the node variable returned by for_each_node will index into
  flow-stats[node].
  
  For example, if node_possible_map is 0x30003, this patch will map node to
  node_cnt as follows:
  0,1,16,17 = 0,1,2,3
  
  The crash was noticed after 3af229f2 was applied as it changed the
  node_possible_map to match node_online_map on boot.
  Fixes: 3af229f2071f5b5cb31664be6109561fbe19c861
 
 My concern with this version of the fix is that you're relying on,
 implicitly, the order of for_each_node's iteration corresponding to the
 entries in stats 1:1. But what about node hotplug? It seems better to
 have the enumeration of the stats array match the topology accurately,
 rather, or to maintain some sort of internal map in the OVS code between
 the NUMA node and the entry in the stats array?
 
 I'm willing to be convinced otherwise, though :)
 
 -Nish


Nish,

The method I described should work for hotplug since it's using possible map
which AFAIK is static rather than the online map. 

Regardless, the more simple solution to solve this issue would be to just
allocate nr_node_ids number of entries and use up extra memory.

I'll send a v2 after testing it.

--chris

  Signed-off-by: Chris J Arges chris.j.ar...@canonical.com
  ---
   net/openvswitch/flow.c   | 10 ++
   net/openvswitch/flow_table.c | 18 +++---
   2 files changed, 17 insertions(+), 11 deletions(-)
  
  diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
  index bc7b0ab..425d45d 100644
  --- a/net/openvswitch/flow.c
  +++ b/net/openvswitch/flow.c
  @@ -134,14 +134,14 @@ void ovs_flow_stats_get(const struct sw_flow *flow,
  struct ovs_flow_stats *ovs_stats,
  unsigned long *used, __be16 *tcp_flags)
   {
  -   int node;
  +   int node, node_cnt = 0;
  
  *used = 0;
  *tcp_flags = 0;
  memset(ovs_stats, 0, sizeof(*ovs_stats));
  
  for_each_node(node) {
  -   struct flow_stats *stats = 
  rcu_dereference_ovsl(flow-stats[node]);
  +   struct flow_stats *stats = 
  rcu_dereference_ovsl(flow-stats[node_cnt]);
  
  if (stats) {
  /* Local CPU may write on non-local stats, so we must
  @@ -155,16 +155,17 @@ void ovs_flow_stats_get(const struct sw_flow *flow,
  ovs_stats-n_bytes += stats-byte_count;
  spin_unlock_bh(stats-lock);
  }
  +   node_cnt++;
  }
   }
  
   /* Called with ovs_mutex. */
   void ovs_flow_stats_clear(struct sw_flow *flow)
   {
  -   int node;
  +   int node, node_cnt = 0;
  
  for_each_node(node) {
  -   struct flow_stats *stats = ovsl_dereference(flow-stats[node]);
  +   struct flow_stats *stats = 
  ovsl_dereference(flow-stats[node_cnt]);
  
  if (stats) {
  spin_lock_bh(stats-lock);
  @@ -174,6 +175,7 @@ void ovs_flow_stats_clear(struct sw_flow *flow)
  stats-tcp_flags = 0;
  spin_unlock_bh(stats-lock);
  }
  +   node_cnt++;
  }
   }
  
  diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
  index 4613df8..5d10c54 100644
  --- a/net/openvswitch/flow_table.c
  +++ b/net/openvswitch/flow_table.c
  @@ -77,7 +77,7 @@ struct sw_flow *ovs_flow_alloc(void)
   {
  struct sw_flow *flow;
  struct flow_stats *stats;
  -   int node;
  +   int node, node_cnt = 0;
  
  flow = kmem_cache_alloc(flow_cache, GFP_KERNEL);
  if (!flow)
  @@ -99,9 +99,11 @@ struct sw_flow *ovs_flow_alloc(void)
  
  RCU_INIT_POINTER(flow-stats[0], stats);
  
  -   for_each_node(node)
  +   for_each_node(node) {
  if (node != 0)
  -   RCU_INIT_POINTER(flow-stats[node], NULL);
  +   RCU_INIT_POINTER(flow-stats[node_cnt], NULL);
  +   node_cnt++;
  +   }
  
  return flow;
   err:
  @@ -139,15 +141,17 @@ static struct flex_array *alloc_buckets(unsigned int 
  n_buckets)
  
   static void flow_free(struct sw_flow *flow)
   {
  -   int node;
  +   int node, node_cnt = 0;
  
  if (ovs_identifier_is_key(flow-id))
  kfree(flow-id.unmasked_key);
  kfree((struct sw_flow_actions __force *)flow-sf_acts);
  -   for_each_node(node)
  -   if (flow-stats[node])
  +   for_each_node(node) {
  +   if (flow-stats[node_cnt

[PATCH v2] openvswitch: allocate nr_node_ids flow_stats instead of num_possible_nodes

[Chris J Arges]

Some architectures like POWER can have a NUMA node_possible_map that
contains sparse entries. This causes memory corruption with openvswitch
since it allocates flow_cache with a multiple of num_possible_nodes() and
assumes the node variable returned by for_each_node will index into
flow-stats[node].

Use nr_node_ids to allocate a maximal sparse array instead of
num_possible_nodes().

The crash was noticed after 3af229f2 was applied as it changed the
node_possible_map to match node_online_map on boot.
Fixes: 3af229f2071f5b5cb31664be6109561fbe19c861

Signed-off-by: Chris J Arges chris.j.ar...@canonical.com
---
 net/openvswitch/flow_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 4613df8..6552394 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -752,7 +752,7 @@ int ovs_flow_init(void)
BUILD_BUG_ON(sizeof(struct sw_flow_key) % sizeof(long));
 
flow_cache = kmem_cache_create(sw_flow, sizeof(struct sw_flow)
-  + (num_possible_nodes()
+  + (nr_node_ids
  * sizeof(struct flow_stats *)),
   0, 0, NULL);
if (flow_cache == NULL)
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

unregister_netdevice warnings when creating/destroying netns

[Chris J Arges]

Hi,

Occasionally starting and stopping many containers with network traffic may
result in new containers being unable to start due to the inability to create
new network namespaces.

This has been reported to happen in kernels up to 4.0.

A reproducer for this issue has been posted here:
  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1403152
To summarize, containers are created in parallel that create a file on an NFS
mount. After this is done, the containers are destroyed. After 10-15 iterations
using 5 worker threads the problem occurs.

Kernel bugzilla entry:
  https://bugzilla.kernel.org/show_bug.cgi?id=81211

The following message repeats in the kernel log until reboot.

  unregister_netdevice: waiting for lo to become free. Usage count = 1

Eventually when creating a new container this hung task backtrace occurs:

  schedule_preempt_disabled+0x29/0x70
  __mutex_lock_slowpath+0x135/0x1b0
  ? __kmalloc+0x1e9/0x230
  mutex_lock+0x1f/0x2f
  copy_net_ns+0x71/0x130
  create_new_namespaces+0xf9/0x180
  copy_namespaces+0x73/0xa0
  copy_process.part.26+0x9a6/0x16b0
  do_fork+0xd5/0x340
  ? call_rcu_sched+0x1d/0x20
  SyS_clone+0x16/0x20
  stub_clone+0x69/0x90
  ? system_call_fastpath+0x1a/0x1f

The following conditions I've been able to test:
- If CONFIG_BRIDGE_NETFILTER is disabled this problem does not occur.
- If net.bridge.bridge-nf-call-iptables is disabled, this problem does not 
occur.
- This problem can happen on single processor machines
- This problem can happen with IPv6 disabled
- This problem can happen with xt_conntrack disabled.
- If NFS uses UDP instead of TCP the problem does not occur.

The unregister_netdevice warning always waits on lo. It always has reg_state
set to NETREG_UNREGISTERING. This follows that the device has been through the
unregister_netdevice_many path and is being unregistered. This path is 
ultimately
where net_mutex is locked and thus prevents copy_net_ns from executing.

In addition, when the unregister netdevice warning happens, a crashdump reveals
the dst_busy_list always contains a dst_entry that references the device above.
This dst_entry has already been through ___dst_free since it has already been
marked DST_OBSOLETE_DEAD. 'dst-ops' is always set to ipv4_dst_ops.
dst-callback_head.next is NULL, and the next pointer is NULL. Use is also zero.

We can trace where the dst_entry is trying to be freed. When free_fib_info_rcu
is called, if nh_rth_input is set, it eventually calls dst_free. Because there
is still a refcnt held, it does not get immediately destroyed and continues on
to __dst_free. This puts the dst into the dst_garbage list, which is then
examined periodically by the dst_gc_work worker thread. Each time it tries to
clean it up it fails because it still has a non-zero refcnt.

The faulty dst_entry is being allocated via ip_rcv..ip_route_input_noref. In
addition this dst is most likely being held in response to a new packet via the
ip_rcv..inet_sk_rx_dst_set path.

At the time of first hitting the 'unregister_netdevice' warning, there are two
sockets that reference dst. Found via 'crash search dst_entry addr'.

Example of sockets that reference the faulty dst entries:

struct inet_sock 88036584f800
  tcp
  rx_dst_ifindex = 150
  skc_state = TCP_CLOSE
  sk_rx_dst = 0x88034a0f5000
  skc_refcnt = 0
  sk_lock.owned = 0
  sk_shutdown = 3
  sk_flags = 17163

struct sock 880427d1fc00
  udp
  skc_state = 1
  sk_rx_dst = 0x88034a0f5000
  skc_refcnt = 0
  sk_lock.owned = 0

Hopefully this gives some context into the issue. I'm happy to do any
additional experiments/debugging since I can reproduce this at will. 

Thanks,
--chris j arges
--
To unsubscribe from this list: send the line unsubscribe netdev in