witness found wanting (was Re: LORs in ipfilter)

In some mail from Giorgos Keramidas, sie said:
> The locking changes of ipfilter have introduced a few LORs, which became
> visible right  after the build fixes  committed by Scott.   Here are the
> ones I've seen so far.
>
> : lock order reversal
> : 1st 0xc072d0a0 ifnet (ifnet) _at_ /usr/src/sys/contrib/ipfilter/netinet/fil.c:2146
> : 2nd 0xc06f9380 ipf IP NAT rwlock (ipf IP NAT rwlock) _at_ /usr/src/sys/contrib/ipfilter/netinet/ip_nat.c:2836
> : KDB: stack backtrace:
> : kdb_backtrace(0,ffffffff,c0708df8,c07083f8,c06d9aac) at kdb_backtrace+0x29
> : witness_checkorder(c06f9380,9,c0676e6c,b14) at witness_checkorder+0x54c
> : _sx_xlock(c06f9380,c0676e6c,b14,3,c1e9a000) at _sx_xlock+0x50
> : ip_natsync(c1e9a000,0,d95f9c84,c0448dd9,0) at ip_natsync+0x20
> : frsync(0,c04f7994,c1d55fac,0,c068949f) at frsync+0x2e
> : iplioctl(c1e98b00,80047249,c1fa09e0,3,c1fba450) at iplioctl+0x551
> : devfs_ioctl_f(c1ff1d48,80047249,c1fa09e0,c1d67d80,c1fba450) at devfs_ioctl_f+0x87
> : ioctl(c1fba450,d95f9d14,3,1,246) at ioctl+0x370
> : syscall(2f,2f,2f,280556c0,bfbfeed4) at syscall+0x213
> : Xint0x80_syscall() at Xint0x80_syscall+0x1f
> : --- syscall (54, FreeBSD ELF32, ioctl), eip = 0x280c67e7, esp = 0xbfbfea7c, ebp = 0xbfbfea98 ---

What exactly is it complaining about ?

The first two LORs in your email are similar - threads that start in
a system call, filter through to the ipfilter code that acquires an
sx lock after the system has acquireqd a mutex.  In neither case does
the mutex provide any protection against what is being achieved by
getting the sx lock.

If there is a bug here, it is in the assumptions that are built into
the "witness code" checking of how locks can be used together.

It may be that these need to be "blessed" (and you use the BLESSED option)
because this behaviour is absolutely required and is not a "bug" or flaw.

What is strange, I find, is that with witness turned on, you have
not reported a panic for the recusrive acquiring of the ifnet lock.
I suppose we're still approaching the point where it is safe to do
that sort of recusive checking on mutexes ?

> Converting the sx locks used by ipfilter to mutexes fixed these LORs but
> introduced a new one, which I'm not sure how to fix:

Which is surprising because it doesn't really fix the problem, in my
eyes, it just highlights the above weakness/bug in the witness code.
It also seriously degrades the performance of ipfilter on an smp system.
That this sort of change makes the witness code silent is further
evidence that it is not really doing its job.

If you like, this is a classic case of bottom/top half driver locking
where shared locks are used, instead of mutexes, to provide the requried
synchronisation.

In summary, what you've reported is not a problem with ipfilter code or
use of locks, as far as I can tell.  I could go on...

Darren