[kamailio/kamailio] dmq_usrloc: fix deadlock (#911)

List overview All Threads
Download

newer

older

git:master:0546af13: dmq_usrloc:...

git:master:311f995f: rtpproxy:...

Julien Chavanton

4 Jan 2017 4 Jan '17

3:39 p.m.

caused by acquiring the same lock twice using get_urecord_by_ruid and lock_udomain You can view, comment on, or merge this pull request online at:

https://github.com/kamailio/kamailio/pull/911

-- Commit Summary --

* dmq_usrloc: fix deadlock

-- File Changes --

M src/modules/dmq_usrloc/usrloc_sync.c (5)

-- Patch Links --

https://github.com/kamailio/kamailio/pull/911.patch https://github.com/kamailio/kamailio/pull/911.diff

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911

Attachments:

attachment.html (text/html — 2.5 KB)

Show replies by date

Julien Chavanton

4 Jan 4 Jan

3:42 p.m.

jchavanton commented on this pull request.

...

@@ -72,6 +70,7 @@ static int add_contact(str aor, ucontact_info_t* ci)

// Search by ruid, if possible res = dmq_ul.get_urecord_by_ruid(_d, dmq_ul.get_aorhash(&aor), &ci->ruid, &r, &c);

`get_urecord_by_ruid` was also acquiring the domain lock, this is why we must do `lock_udomain` only after it returns

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911#pullrequestreview-15134761

Julien Chavanton

3:59 p.m.

While running the CI tests Kamctl / MI fifo was hanging, after further investigation it seems it was trying to acquire a lock while the DMK module was stuck using it.

Processes ``` Process:: ID=0 PID=6 Type=main process - attendant Process:: ID=1 PID=168 Type=udp receiver child=0 sock=0.0.0.0:5060 ! (172.19.0.12:5060) ! Process:: ID=2 PID=169 Type=udp receiver child=1 sock=0.0.0.0:5060 (172.19.0.12:5060) ! Process:: ID=3 PID=170 Type=slow timer Process:: ID=4 PID=171 Type=timer Process:: ID=5 PID=172 Type=secondary timer Process:: ID=6 PID=173 Type=MI FIFO Process:: ID=7 PID=174 Type=ctl handler ! Process:: ID=8 PID=175 Type=RTIMER SEC EXEC Process:: ID=9 PID=176 Type=RTIMER SEC EXEC Process:: ID=10 PID=177 Type=HTable Timer Process:: ID=11 PID=178 Type=USRLOC Timer Process:: ID=12 PID=179 Type=USRLOC Timer Process:: ID=13 PID=180 Type=TIMER NH Process:: ID=14 PID=181 Type=TIMER NH Process:: ID=15 PID=182 Type=DMQ WORKER Process:: ID=16 PID=183 Type=DMQ WORKER Process:: ID=17 PID=184 Type=tcp main process ```

gdb /usr/sbin/kamailio 173 ``` (gdb) bt #0 0x0000785ee4f2f042 in __syscall6 (a6=<optimized out>, a5=<optimized out>, a4=<optimized out>, a3=<optimized out>, a2=<optimized out>, a1=<optimized out>, n=202) at ./arch/x86_64/syscall_arch.h:59 #1 syscall (n=n@entry=202) at src/misc/syscall.c:18 #2 0x0000785eb6ec670b in futex_get (lock=0x785eb79215f4) at ../../lib/kcore/../../mem/../futexlock.h:108 #3 0x0000785eb6ecc200 in lock_ulslot (_d=_d@entry=0x785eb7955690, i=i@entry=472189) at udomain.c:1077 #4 0x0000785eb6ec1947 in mi_usrloc_dump (cmd=<optimized out>, param=<optimized out>) at ul_mi.c:451 #5 0x0000785edc4c0fa1 in run_mi_cmd (t=0xa9bbcf24100, cmd=0xa9bbce892e0) at ../../lib/kmi/mi.h:77 #6 mi_fifo_server (fifo_stream=fifo_stream@entry=0xa9bbce8a5a0) at fifo_fnc.c:509 #7 0x0000785edc4c3317 in fifo_process (rank=rank@entry=1) at mi_fifo.c:270 #8 0x0000785edc4c53b4 in mi_child_init (rank=<optimized out>) at mi_fifo.c:234 #9 0x00000a9bb8e9b6e6 in init_mod_child (m=0x785edcf22008, rank=rank@entry=0) at sr_module.c:921 #10 0x00000a9bb8e9b4f0 in init_mod_child (m=0x785edcf22270, rank=rank@entry=0) at sr_module.c:918 #11 0x00000a9bb8e9b4f0 in init_mod_child (m=0x785edcf227f0, rank=rank@entry=0) at sr_module.c:918 #12 0x00000a9bb8e9b4f0 in init_mod_child (m=0x785edcf22d40, rank=rank@entry=0) at sr_module.c:918 #13 0x00000a9bb8e9b4f0 in init_mod_child (m=0x785edcf246d0, rank=rank@entry=0) at sr_module.c:918 #14 0x00000a9bb8e9b4f0 in init_mod_child (m=0x785edcf252f8, rank=rank@entry=0) at sr_module.c:918 ```

gdb /usr/sbin/kamailio 182 ``` (gdb) bt #0 0x0000785ee4f2f042 in __syscall6 (a6=<optimized out>, a5=<optimized out>, a4=<optimized out>, a3=<optimized out>, a2=<optimized out>, a1=<optimized out>, n=202) at ./arch/x86_64/syscall_arch.h:59 #1 syscall (n=n@entry=202) at src/misc/syscall.c:18 #2 0x0000785eb6ec674e in futex_get (lock=0x785eb79215f4) at ../../lib/kcore/../../mem/../futexlock.h:121 #3 0x0000785eb6ece5f7 in get_urecord_by_ruid (_d=0x785eb7955690, _aorhash=1819751549, _ruid=_ruid@entry=0x785eb68201c0 <ci>, _r=_r@entry=0x7d7464b7e400, _c=_c@entry=0x7d7464b7e410) at udomain.c:1178 #4 0x0000785eb66187b3 in add_contact (ci=0x785eb68201c0 <ci>, aor=...) at usrloc_sync.c:72 #5 usrloc_dmq_handle_msg (msg=<optimized out>, resp=0x7d7464b7e520, node=0x785eb8d5b418) at usrloc_sync.c:429 #6 0x0000785eb682adcc in worker_loop (id=id@entry=0) at worker.c:105 #7 0x0000785eb682c56b in child_init (rank=<optimized out>) at dmq.c:293 #8 0x00000a9bb8e9b6e6 in init_mod_child (m=0x785edcf9fe78, rank=rank@entry=0) at sr_module.c:921 #9 0x00000a9bb8e9b4f0 in init_mod_child (m=0x785edcfa02e0, rank=0) at sr_module.c:918 #10 0x00000a9bb8eacbde in main_loop () at main.c:1682 #11 0x00000a9bb8e6249b in main (argc=<optimized out>, argv=<optimized out>) at main.c:2616 ```

gdb /usr/sbin/kamailio 183 ``` (gdb) bt #0 0x0000785ee4f2f042 in __syscall6 (a6=<optimized out>, a5=<optimized out>, a4=<optimized out>, a3=<optimized out>, a2=<optimized out>, a1=<optimized out>, n=202) at ./arch/x86_64/syscall_arch.h:59 #1 syscall (n=n@entry=202) at src/misc/syscall.c:18 #2 0x0000785eb6ec670b in futex_get (lock=0x785eb79215f4) at ../../lib/kcore/../../mem/../futexlock.h:108 #3 0x0000785eb661807c in add_contact (ci=0x785eb68201c0 <ci>, aor=...) at usrloc_sync.c:64 #4 usrloc_dmq_handle_msg (msg=<optimized out>, resp=0x7d7464b7e520, node=0x785eb8d5b418) at usrloc_sync.c:429 #5 0x0000785eb682adcc in worker_loop (id=id@entry=1) at worker.c:105 #6 0x0000785eb682c56b in child_init (rank=<optimized out>) at dmq.c:293 #7 0x00000a9bb8e9b6e6 in init_mod_child (m=0x785edcf9fe78, rank=rank@entry=0) at sr_module.c:921 #8 0x00000a9bb8e9b4f0 in init_mod_child (m=0x785edcfa02e0, rank=0) at sr_module.c:918 #9 0x00000a9bb8eacbde in main_loop () at main.c:1682 #10 0x00000a9bb8e6249b in main (argc=<optimized out>, argv=<optimized out>) at main.c:2616 ```

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911#issuecomment-270406869

Daniel-Constantin Mierla

4:03 p.m.

No time to look in the code, but if it does two times lock and only one time unlock, then it is indeed a deadlock. If any other devel using dmq_usrloc does the review, then it can be committed. If not, I will check when I get a bit of time.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911#issuecomment-270408338

Julien Chavanton

6:31 p.m.

The same process is requesting the lock a second time before unlocking. `futex_get()` is never able to acquire the lock and does not return, I tested this in a very minimalist program using `pthread_mutex_lock()` and it does get stuck.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911#issuecomment-270447814

Daniel-Constantin Mierla

5 Jan 5 Jan

7:39 a.m.

miconda commented on this pull request.

...

@@ -72,6 +70,7 @@ static int add_contact(str aor, ucontact_info_t* ci)

// Search by ruid, if possible res = dmq_ul.get_urecord_by_ruid(_d, dmq_ul.get_aorhash(&aor), &ci->ruid, &r, &c); if (res == 0) { + dmq_ul.lock_udomain(_d, &aor);

This addition to line 73 doesn't seem necessary, because the lock is aquired by get_urecord_by_ruid(). Or did I miss something?

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911#pullrequestreview-15258004

Julien Chavanton

2:42 p.m.

@jchavanton pushed 1 commit.

309873e dmq_usrloc: get_urecord_by_ruid returns

-- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/kamailio/kamailio/pull/911/files/9af42f16badfe6e1949cd42b...

Julien Chavanton

2:42 p.m.

jchavanton commented on this pull request.

...

@@ -72,6 +70,7 @@ static int add_contact(str aor, ucontact_info_t* ci)

// Search by ruid, if possible res = dmq_ul.get_urecord_by_ruid(_d, dmq_ul.get_aorhash(&aor), &ci->ruid, &r, &c); if (res == 0) { + dmq_ul.lock_udomain(_d, &aor);

You are right, if the record is found the lock is kept.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911

Daniel-Constantin Mierla

3:32 p.m.

Merged #911.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/911#event-912795700

3090

Age (days ago)

3091

Last active (days ago)

sr-dev@lists.kamailio.org

8 comments

2 participants

tags (0)

participants (2)

Daniel-Constantin Mierla
Julien Chavanton