ip_fw: address lock order reversal

[rscheff]

Summary

Infrequently, instrumented kernel would complain about a lock order reversal
when obtaining locks in ip_fw2.c. Maintain lock ordering by tracking
any nested locking using a flag, and then executing the locking in the expected sequence.

Reported by: Jimmy Zhang
Obtained from: Jimmy Zhang
Sponsored by: NetApp, Inc.

[ae]

sys/netpfil/ipfw/ip_fw2.c

3390

This does not seem correct. I think it should not compile.
cmd_copy.arg1 is not pointer to struct ipfw_insn. And even if it would be, this instruction has variable length and ipfw_insn is not enough to keep it.

static void
send_reject(struct ip_fw_args *args, const ipfw_insn *cmd, int iplen,
    struct ip *ip)

[ae]

I think this patch should do what you need.

[kib]

What do you mean by 'lock release ordering'? Can you show exact diagnostic you are trying to fix, please?

[rscheff]

In D48069#1095714, @kib wrote:

What do you mean by 'lock release ordering'? Can you show exact diagnostic you are trying to fix, please?

lock order reversal:
1st 0xfffffe10d700f978 IPFW static rules (IPFW static rules, rm) @ src/sys/netpfil/ipfw/ip_fw2.c:1908
2nd 0xffffffff81bbb888 rawcb (rawcb, sleep mutex) @ src/sys/netipsec/keysock.c:208
lock order IPFW static rules -> rawcb attempted at:
#0 0xffffffff80acced5 at witness_checkorder+0x1265
#1 0xffffffff809e3327 at __mtx_lock_flags+0x2d7
#2 0xffffffff80d8a66b at key_sendup_mbuf+0x21b
#3 0xffffffff80d69ccb at key_allocsa_policy_debug+0xb2b
#4 0xffffffff80d6a018 at key_allocsa_policy+0x2d8
#5 0xffffffff80d65175 at ipsec4_check_pmtu+0x535
#6 0xffffffff80d648d5 at ipsec4_process_packet+0xb5
#7 0xffffffff80d6485a at ipsec4_process_packet+0x3a
#8 0xffffffff80d65582 at ipsec4_output+0x172
#9 0xffffffff80d6546d at ipsec4_output+0x5d
#10 0xffffffff80c8d16b at ip_output+0x288b
#11 0xffffffff80da074b at ipfw_chk+0xa41b
#12 0xffffffff80d9dfd7 at ipfw_chk+0x7ca7
#13 0xffffffff80db10bf at ipfw_chg_hook+0xb8f
#14 0xffffffff80c3fc9f at pfil_run_hooks+0x16f
#15 0xffffffff80c8533d at ip_input+0xc0d

[rscheff]

• only keep relevant reject code and mtu rather than full ipfw_insn struct

[rscheff]

In D48069#1095696, @ae wrote:

I think this patch should do what you need.

Thanks! I updated this diff accordingly; even before that, the compile would complete, but it is definitely cleaner to only keep the actual required simple values around and not use the stack to store a variable size struct.

[kib]

In D48069#1095720, @rscheff wrote:

In D48069#1095714, @kib wrote:

What do you mean by 'lock release ordering'? Can you show exact diagnostic you are trying to fix, please?

lock order reversal:
1st 0xfffffe10d700f978 IPFW static rules (IPFW static rules, rm) @ src/sys/netpfil/ipfw/ip_fw2.c:1908
2nd 0xffffffff81bbb888 rawcb (rawcb, sleep mutex) @ src/sys/netipsec/keysock.c:208
lock order IPFW static rules -> rawcb attempted at:
#0 0xffffffff80acced5 at witness_checkorder+0x1265
#1 0xffffffff809e3327 at __mtx_lock_flags+0x2d7
#2 0xffffffff80d8a66b at key_sendup_mbuf+0x21b
#3 0xffffffff80d69ccb at key_allocsa_policy_debug+0xb2b
#4 0xffffffff80d6a018 at key_allocsa_policy+0x2d8
#5 0xffffffff80d65175 at ipsec4_check_pmtu+0x535
#6 0xffffffff80d648d5 at ipsec4_process_packet+0xb5
#7 0xffffffff80d6485a at ipsec4_process_packet+0x3a
#8 0xffffffff80d65582 at ipsec4_output+0x172
#9 0xffffffff80d6546d at ipsec4_output+0x5d
#10 0xffffffff80c8d16b at ip_output+0x288b
#11 0xffffffff80da074b at ipfw_chk+0xa41b
#12 0xffffffff80d9dfd7 at ipfw_chk+0x7ca7
#13 0xffffffff80db10bf at ipfw_chg_hook+0xb8f
#14 0xffffffff80c3fc9f at pfil_run_hooks+0x16f
#15 0xffffffff80c8533d at ip_input+0xc0d

So it is about locking, and not unlocking.

[ae]

I didn't test the patch, so if it is works for you I have no objection. :-)

[glebius]

sys/netpfil/ipfw/ip_fw2.c

996–997

If those arguments are exactly what is goint to be put into ICMP, let's make them uint8_t code, uint16_t mtu. The struct ip * can also be made const pointer. Similar changes can be done to send_reject6(), too.

[rscheff]

• update int types used in function header
• proliferate the const struct to avoid compiler warnings

[glebius]

uint8_t code

Oh shi, ip_fw.h declares fake ICMP codes that are > 0xff. :( Well, fixing that is out of scope of this review.