From: Saeed Mahameed <saeed@kernel.org>
To: Leon Romanovsky <leon@kernel.org>
Cc: Ying Hsu <yinghsu@chromium.org>,
linux-bluetooth@vger.kernel.org,
chromeos-bluetooth-upstreaming@chromium.org,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>,
Johan Hedberg <johan.hedberg@gmail.com>,
Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
Marcel Holtmann <marcel@holtmann.org>,
Paolo Abeni <pabeni@redhat.com>,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH] Bluetooth: Fix possible deadlock in rfcomm_sk_state_change
Date: Thu, 5 Jan 2023 16:24:10 -0800 [thread overview]
Message-ID: <Y7dqKnQe8UUeQ/CD@x130> (raw)
In-Reply-To: <Y7UiDn3Gi5YdNIoC@unreal>
On 04 Jan 08:51, Leon Romanovsky wrote:
>On Tue, Jan 03, 2023 at 11:12:46AM +0000, Ying Hsu wrote:
>> There's a possible deadlock when two processes are connecting
>> and closing concurrently:
>> + CPU0: __rfcomm_dlc_close locks rfcomm and then calls
>> rfcomm_sk_state_change which locks the sock.
>> + CPU1: rfcomm_sock_connect locks the socket and then calls
>> rfcomm_dlc_open which locks rfcomm.
>>
>> Here's the call trace:
>>
>> -> #2 (&d->lock){+.+.}-{3:3}:
>> __mutex_lock_common kernel/locking/mutex.c:603 [inline]
>> __mutex_lock0x12f/0x1360 kernel/locking/mutex.c:747
>> __rfcomm_dlc_close+0x15d/0x890 net/bluetooth/rfcomm/core.c:487
>> rfcomm_dlc_close+1e9/0x240 net/bluetooth/rfcomm/core.c:520
>> __rfcomm_sock_close+0x13c/0x250 net/bluetooth/rfcomm/sock.c:220
>> rfcomm_sock_shutdown+0xd8/0x230 net/bluetooth/rfcomm/sock.c:907
>> rfcomm_sock_release+0x68/0x140 net/bluetooth/rfcomm/sock.c:928
>> __sock_release+0xcd/0x280 net/socket.c:650
>> sock_close+0x1c/0x20 net/socket.c:1365
>> __fput+0x27c/0xa90 fs/file_table.c:320
>> task_work_run+0x16f/0x270 kernel/task_work.c:179
>> exit_task_work include/linux/task_work.h:38 [inline]
>> do_exit+0xaa8/0x2950 kernel/exit.c:867
>> do_group_exit+0xd4/0x2a0 kernel/exit.c:1012
>> get_signal+0x21c3/0x2450 kernel/signal.c:2859
>> arch_do_signal_or_restart+0x79/0x5c0 arch/x86/kernel/signal.c:306
>> exit_to_user_mode_loop kernel/entry/common.c:168 [inline]
>> exit_to_user_mode_prepare+0x15f/0x250 kernel/entry/common.c:203
>> __syscall_exit_to_user_mode_work kernel/entry/common.c:285 [inline]
>> syscall_exit_to_user_mode+0x1d/0x50 kernel/entry/common.c:296
>> do_syscall_64+0x46/0xb0 arch/x86/entry/common.c:86
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> -> #1 (rfcomm_mutex){+.+.}-{3:3}:
>> __mutex_lock_common kernel/locking/mutex.c:603 [inline]
>> __mutex_lock+0x12f/0x1360 kernel/locking/mutex.c:747
>> rfcomm_dlc_open+0x93/0xa80 net/bluetooth/rfcomm/core.c:425
>> rfcomm_sock_connect+0x329/0x450 net/bluetooth/rfcomm/sock.c:413
>> __sys_connect_file+0x153/0x1a0 net/socket.c:1976
>> __sys_connect+0x165/0x1a0 net/socket.c:1993
>> __do_sys_connect net/socket.c:2003 [inline]
>> __se_sys_connect net/socket.c:2000 [inline]
>> __x64_sys_connect+0x73/0xb0 net/socket.c:2000
>> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> -> #0 (sk_lock-AF_BLUETOOTH-BTPROTO_RFCOMM){+.+.}-{0:0}:
>> check_prev_add kernel/locking/lockdep.c:3097 [inline]
>> check_prevs_add kernel/locking/lockdep.c:3216 [inline]
>> validate_chain kernel/locking/lockdep.c:3831 [inline]
>> __lock_acquire+0x2a43/0x56d0 kernel/locking/lockdep.c:5055
>> lock_acquire kernel/locking/lockdep.c:5668 [inline]
>> lock_acquire+0x1e3/0x630 kernel/locking/lockdep.c:5633
>> lock_sock_nested+0x3a/0xf0 net/core/sock.c:3470
>> lock_sock include/net/sock.h:1725 [inline]
>> rfcomm_sk_state_change+0x6d/0x3a0 net/bluetooth/rfcomm/sock.c:73
>> __rfcomm_dlc_close+0x1b1/0x890 net/bluetooth/rfcomm/core.c:489
>> rfcomm_dlc_close+0x1e9/0x240 net/bluetooth/rfcomm/core.c:520
>> __rfcomm_sock_close+0x13c/0x250 net/bluetooth/rfcomm/sock.c:220
>> rfcomm_sock_shutdown+0xd8/0x230 net/bluetooth/rfcomm/sock.c:907
>> rfcomm_sock_release+0x68/0x140 net/bluetooth/rfcomm/sock.c:928
>> __sock_release+0xcd/0x280 net/socket.c:650
>> sock_close+0x1c/0x20 net/socket.c:1365
>> __fput+0x27c/0xa90 fs/file_table.c:320
>> task_work_run+0x16f/0x270 kernel/task_work.c:179
>> exit_task_work include/linux/task_work.h:38 [inline]
>> do_exit+0xaa8/0x2950 kernel/exit.c:867
>> do_group_exit+0xd4/0x2a0 kernel/exit.c:1012
>> get_signal+0x21c3/0x2450 kernel/signal.c:2859
>> arch_do_signal_or_restart+0x79/0x5c0 arch/x86/kernel/signal.c:306
>> exit_to_user_mode_loop kernel/entry/common.c:168 [inline]
>> exit_to_user_mode_prepare+0x15f/0x250 kernel/entry/common.c:203
>> __syscall_exit_to_user_mode_work kernel/entry/common.c:285 [inline]
>> syscall_exit_to_user_mode+0x1d/0x50 kernel/entry/common.c:296
>> do_syscall_64+0x46/0xb0 arch/x86/entry/common.c:86
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Signed-off-by: Ying Hsu <yinghsu@chromium.org>
>> ---
>> This commit has been tested with a C reproducer on qemu-x86_64.
>>
>> net/bluetooth/rfcomm/sock.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
>> index 21e24da4847f..29f9a88a3dc8 100644
>> --- a/net/bluetooth/rfcomm/sock.c
>> +++ b/net/bluetooth/rfcomm/sock.c
>> @@ -410,8 +410,10 @@ static int rfcomm_sock_connect(struct socket *sock, struct sockaddr *addr, int a
>> d->sec_level = rfcomm_pi(sk)->sec_level;
>> d->role_switch = rfcomm_pi(sk)->role_switch;
>>
>> + release_sock(sk);
>> err = rfcomm_dlc_open(d, &rfcomm_pi(sk)->src, &sa->rc_bdaddr,
> ^^^^
>Are you sure that "sk" still exists here after you called to release_sock(sk)?
>What prevents from use-after-free here?
>
sk must be valid to be locked in first place.
release_sock() has mutex unlock semantics so it doesn't free anything..
from a quick review it doesn't seem to be necessary to keep the lock held
around rfcomm_dlc_open, and looking into bt_sock_wait_state, it also
releases the lock temporarily internally .. so it won't matter if the lock
got released temporarily earlier, IMHO.
Though you might want to make sure rfcomm_pi(sk)->src won't change,
it seems like other functions use &rfcomm_sk_list.lock to protect it.
Reviewed-by: Saeed Mahameed <saeed@kernel.org>
>> sa->rc_channel);
>> + lock_sock(sk);
>> if (!err)
>> err = bt_sock_wait_state(sk, BT_CONNECTED,
>> sock_sndtimeo(sk, flags & O_NONBLOCK));
>> --
>> 2.39.0.314.g84b9a713c41-goog
>>
next prev parent reply other threads:[~2023-01-06 0:24 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-03 11:12 [PATCH] Bluetooth: Fix possible deadlock in rfcomm_sk_state_change Ying Hsu
2023-01-03 11:57 ` bluez.test.bot
2023-01-04 6:51 ` [PATCH] " Leon Romanovsky
2023-01-06 0:24 ` Saeed Mahameed [this message]
2023-01-08 10:12 ` Leon Romanovsky
2023-01-10 9:07 ` Saeed Mahameed
2023-01-10 12:22 ` Leon Romanovsky
2023-01-11 1:03 ` Saeed Mahameed
2023-01-11 0:34 ` Luiz Augusto von Dentz
2023-01-11 0:50 ` Saeed Mahameed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y7dqKnQe8UUeQ/CD@x130 \
--to=saeed@kernel.org \
--cc=chromeos-bluetooth-upstreaming@chromium.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=johan.hedberg@gmail.com \
--cc=kuba@kernel.org \
--cc=leon@kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=yinghsu@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.