Comment 1 Kristof Provost 2017-01-20 21:47:44 UTC

Tests with WITNESS show:

xn0: performing interface reset due to feature change
uma_zalloc_arg: zone "64" with the following non-sleepable locks held:
exclusive sleep mutex if_bridge (if_bridge) r = 0 (0xfffff80003f0aa18) locked @ /usr/home/rootbsd/freebsd/sys/modules/if_bridge/../../net/if_bridge.c:826
stack backtrace:
#0 0xffffffff80a66720 at witness_debugger+0x70
#1 0xffffffff80a679c7 at witness_warn+0x3d7
#2 0xffffffff80cb00bb at uma_zalloc_arg+0x3b
#3 0xffffffff809f328c at malloc+0xfc
#4 0xffffffff80a57c45 at sbuf_new+0x95
#5 0xffffffff808eea18 at xs_rm+0x28
#6 0xffffffff808eb411 at xn_ioctl+0x211
#7 0xffffffff8221d4b3 at bridge_ioctl_add+0x4b3
#8 0xffffffff8221ef8f at bridge_ioctl+0x29f
#9 0xffffffff80ae577a at ifioctl+0x104a
#10 0xffffffff80a6ab64 at kern_ioctl+0x214
#11 0xffffffff80a6a8e1 at sys_ioctl+0x171
#12 0xffffffff80e35fd4 at amd64_syscall+0x314
#13 0xffffffff80e1c1ab at Xfast_syscall+0xfb

Comment 2 Kristof Provost 2017-01-20 22:18:05 UTC

I think what happens here is that the bridge code (through bridge_ioctl_add() calls the xen driver's ioctl() handler for SIOCSIFCAP, which through xn_ioctl() calls xs_rm(xenbus_get_node(dev), "feature-gso-tcp4"), which tries to compose a string with the sbuf functions, which use a M_WAITOK allocation.

That means that we can end up sleeping (because malloc(M_WAITOK)) with the bridge lock (a standard mutex) held.
That violates locking rules, by sleeping with a mutex held, so WITNESS warns us about this.

If we're unlucky enough to actually try to acquire the bridge lock from another thread (say because we want to transmit a packet) we can end up panic()ing.

It's not obvious to me how this can be fixed however. I'm cc-ing royger because he touched the xen-netfront code at some point.

Perhaps we can allocate the strings the xs_rm() needs at device initialisation time, but that would require the result of xenbus_get_node(dev) to be constant, and I don't know if that's a valid assumption.

Comment 3 Kristof Provost 2017-01-22 19:25:33 UTC

Proposed patch: https://reviews.freebsd.org/D9290

Comment 4 commit-hook 2017-01-25 21:26:27 UTC

A commit references this bug:

Author: kp
Date: Wed Jan 25 21:25:26 UTC 2017
New revision: 312782
URL: https://svnweb.freebsd.org/changeset/base/312782

Log:
  bridge: Release the bridge lock when calling bridge_set_ifcap()

  This calls ioctl() handlers for the different interfaces in the bridge.
  These handlers expect to get called in an ioctl context where it's safe
  for them to sleep. We may not sleep with the bridge lock held.

  However, we still need to protect the interface list, to ensure it
  doesn't get changed while we iterate over it.
  Use BRIDGE_XLOCK(), which prevents bridge members from being removed.
  Adding bridge members is safe, because it uses LIST_INSERT_HEAD().

  This caused panics when adding xen interfaces to a bridge.

  PR:		216304
  Reviewed by:	ae
  MFC after:	1 week
  Sponsored by:	RootBSD
  Differential Revision:	https://reviews.freebsd.org/D9290

Changes:
  head/sys/net/if_bridge.c
  head/sys/net/if_bridgevar.h

Hi,

I am getting something similar and I am not sure whether it's related.

> panic: _mtx_lock_sleep: recursed on non-recursive mutex if_bridge @ /usr/src/sys/modules/if_bridge/../../net/if_bridge.c:2117
> 
> cpuid = 1
> KDB: stack backtrace:
> db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00954a3260
> vpanic() at vpanic+0x186/frame 0xfffffe00954a32e0
> kassert_panic() at kassert_panic+0x126/frame 0xfffffe00954a3350
> __mtx_lock_sleep() at __mtx_lock_sleep+0x3e2/frame 0xfffffe00954a33d0
> __mtx_lock_flags() at __mtx_lock_flags+0x116/frame 0xfffffe00954a3430
> bridge_transmit() at bridge_transmit+0x61/frame 0xfffffe00954a3470
> ether_output() at ether_output+0x703/frame 0xfffffe00954a3510
> arprequest() at arprequest+0x413/frame 0xfffffe00954a3620
> arp_ifinit() at arp_ifinit+0x56/frame 0xfffffe00954a3660
> arp_handle_ifllchange() at arp_handle_ifllchange+0x3d/frame 0xfffffe00954a3680
> bridge_ioctl_add() at bridge_ioctl_add+0x3b4/frame 0xfffffe00954a36d0
> bridge_ioctl() at bridge_ioctl+0x29f/frame 0xfffffe00954a3770
> ifioctl() at ifioctl+0xbc8/frame 0xfffffe00954a37f0
> kern_ioctl() at kern_ioctl+0x2b0/frame 0xfffffe00954a3850
> sys_ioctl() at sys_ioctl+0x13f/frame 0xfffffe00954a3930
> amd64_syscall() at amd64_syscall+0x2f9/frame 0xfffffe00954a3ab0
> Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe00954a3ab0
> --- syscall (54, FreeBSD ELF64, sys_ioctl), rip = 0x800fd5aaa, rsp = 0x7fffffffe1f8, rbp = 0x7fffffffe2a0 ---
> KDB: enter: panic
> [ thread pid 770 tid 100560 ]
> Stopped at      kdb_enter+0x3b: movq    $0,kdb_why

Comment 6 Kristof Provost 2017-01-26 21:10:52 UTC

(In reply to nvass from comment #5)
That looks like a different problem. Can you file a new bug with the backtrace and a description of how you reproduce this? Please cc me on it.

Thanks for the quick reply!

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216510

Comment 8 commit-hook 2017-02-01 20:27:45 UTC

A commit references this bug:

Author: kp
Date: Wed Feb  1 20:27:38 UTC 2017
New revision: 313050
URL: https://svnweb.freebsd.org/changeset/base/313050

Log:
  MFC 312782

  bridge: Release the bridge lock when calling bridge_set_ifcap()

  This calls ioctl() handlers for the different interfaces in the bridge.
  These handlers expect to get called in an ioctl context where it's safe
  for them to sleep. We may not sleep with the bridge lock held.

  However, we still need to protect the interface list, to ensure it
  doesn't get changed while we iterate over it.
  Use BRIDGE_XLOCK(), which prevents bridge members from being removed.
  Adding bridge members is safe, because it uses LIST_INSERT_HEAD().

  This caused panics when adding xen interfaces to a bridge.

  PR:		216304
  Reviewed by:	ae
  Sponsored by:	RootBSD
  Differential Revision:	https://reviews.freebsd.org/D9290

Changes:
_U  stable/11/
  stable/11/sys/net/if_bridge.c
  stable/11/sys/net/if_bridgevar.h

Comment 9 commit-hook 2017-02-01 21:45:58 UTC

A commit references this bug:

Author: kp
Date: Wed Feb  1 21:44:50 UTC 2017
New revision: 313066
URL: https://svnweb.freebsd.org/changeset/base/313066

Log:
  MFC 312782

  bridge: Release the bridge lock when calling bridge_set_ifcap()

  This calls ioctl() handlers for the different interfaces in the bridge.
  These handlers expect to get called in an ioctl context where it's safe
  for them to sleep. We may not sleep with the bridge lock held.

  However, we still need to protect the interface list, to ensure it
  doesn't get changed while we iterate over it.
  Use BRIDGE_XLOCK(), which prevents bridge members from being removed.
  Adding bridge members is safe, because it uses LIST_INSERT_HEAD().

  This caused panics when adding xen interfaces to a bridge.

  PR:		216304
  Reviewed by:	ae
  Sponsored by:	RootBSD
  Differential Revision:	https://reviews.freebsd.org/D9290

Changes:
_U  stable/10/
  stable/10/sys/net/if_bridge.c
  stable/10/sys/net/if_bridgevar.h