Page MenuHomeFreeBSD
Differential D48069

ip_fw: address lock order reversal
ClosedPublic
Actions

Authored by rscheff on Dec 13 2024, 9:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Sep 8, 4:04 AM
Unknown Object (File)
Sat, Sep 5, 6:57 PM
Unknown Object (File)
Sat, Sep 5, 6:57 PM
Unknown Object (File)
Sat, Sep 5, 2:38 PM
Unknown Object (File)
Sat, Sep 5, 2:36 PM
Unknown Object (File)
Sat, Sep 5, 1:41 PM
Unknown Object (File)
Fri, Sep 4, 2:32 PM
Unknown Object (File)
Thu, Sep 3, 1:24 AM
View All Files
Subscribers
donner
imp
melifaro
vegeta_tuxpowered.net

Details

Reviewers
glebius
cc
tuexen
thj
kib
ae
jhb
kp
Commits
rG8e7802851e6c: ip_fw: address lock order reversal
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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 61205
Build 58089: arc lint + arc unit

Event Timeline

rscheff created this revision.Dec 13 2024, 9:56 AM
Herald added subscribers: vegeta_tuxpowered.net, donner, melifaro, imp. · View Herald TranscriptDec 13 2024, 9:56 AM
rscheff requested review of this revision.Dec 13 2024, 9:56 AM
Harbormaster completed remote builds in B61140: Diff 147915.Dec 13 2024, 9:56 AM
ae requested changes to this revision.Dec 13 2024, 12:17 PM
ae added inline comments.
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)
This revision now requires changes to proceed.Dec 13 2024, 12:17 PM
ae added a comment.Dec 13 2024, 1:16 PM

I think this patch should do what you need.

kib added a comment.Dec 13 2024, 2:26 PM

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

rscheff added a comment.Dec 13 2024, 2:41 PM
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 updated this revision to Diff 147922.Dec 13 2024, 2:42 PM
  • only keep relevant reject code and mtu rather than full ipfw_insn struct
Harbormaster completed remote builds in B61145: Diff 147922.Dec 13 2024, 2:43 PM
rscheff added a comment.Dec 13 2024, 2:44 PM
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 added a comment.Dec 13 2024, 2:48 PM
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.

rscheff retitled this revision from ip_fw: address (un)lock order reversal to ip_fw: address lock order reversal.Dec 13 2024, 2:58 PM
rscheff edited the summary of this revision. (Show Details)
ae accepted this revision.Dec 13 2024, 5:33 PM

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

This revision is now accepted and ready to land.Dec 13 2024, 5:33 PM
glebius added inline comments.Dec 16 2024, 3:02 PM
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 updated this revision to Diff 148119.Dec 16 2024, 7:48 PM
  • update int types used in function header
  • proliferate the const struct to avoid compiler warnings
This revision now requires review to proceed.Dec 16 2024, 7:48 PM
Harbormaster completed remote builds in B61205: Diff 148119.Dec 16 2024, 7:48 PM
glebius added a comment.Dec 17 2024, 9:52 PM

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.

glebius accepted this revision.Dec 17 2024, 9:53 PM
This revision is now accepted and ready to land.Dec 17 2024, 9:53 PM
Closed by commit rG8e7802851e6c: ip_fw: address lock order reversal (authored by rscheff). · Explain WhyDec 19 2024, 3:39 PM
This revision was automatically updated to reflect the committed changes.
rscheff added a commit: rG8e7802851e6c: ip_fw: address lock order reversal.

Revision Contents
Changeset List

PathSize
sys/
netpfil/
ipfw/
71 lines

Diff 148119

View Options

sys/netpfil/ipfw/ip_fw2.c

Loading...