Re: [PATCH v3] Bluetooth: RFCOMM: connect the session socket without rfcomm_mutex

[Mikhail Gavrilov]

Hi Luiz,

> We might need to check if the sock hasn't been closed in the process:
>
> https://sashiko.dev/#/patchset/20260912100315.151674-1-mikhail.v.gavrilov%40gmail.com

You are right, and the review is right about where it goes wrong.

While the socket is being connected the DLC is not on any session yet, so
a concurrent close is a no-op: rfcomm_dlc_close() leaves without touching
it at all.

s = d->session;
if (!s)
    goto no_session;

d->state therefore stays BT_OPEN, the state check in __rfcomm_dlc_open()
passes when rfcomm_mutex is taken again, and the DLC gets linked to the
new session even though its socket is gone.

The window is not new - rfcomm_sock_connect() drops the sock lock before
calling rfcomm_dlc_open() - but it used to be as short as one mutex
acquisition, and this patch stretches it across a page attempt, so it is
seconds wide now.

The consequence the review points at looks real to me as well.  For an
orphaned DLC rfcomm_sk_data_ready() returns without freeing the skb:

struct sock *sk = d->owner;
if (!sk)
    return;

and rfcomm_recv_data() does not free it either - it calls d->data_ready()
and returns 0, while kfree_skb() sits on the drop: label it never
reaches.  So every data frame the remote sends on such a DLC leaks one
skb.  That leak is reachable whenever a DLC outlives its socket and is
not caused by this patch, but this patch makes it easy to reach, and it
may be worth fixing on its own.

For v4 I plan to make the close visible instead of inferring it from the
state:

 - rfcomm_dlc_close() sets a flag on the DLC in the no-session case, so
   that a close during the window leaves a trace;
 - rfcomm_dlc_open() clears that flag while it still holds rfcomm_mutex
   the first time, and checks it after re-acquiring the lock: if it is
   set, the socket is released and the DLC is not linked;
 - the state check cannot be used for this, since BT_CLOSED is a legal
   starting state for a DLC that is being reused.

Does that match what you had in mind, or would you rather see the owner
pinned for the duration of the connect?  For the socket path the caller
holds the fd across the syscall, but rfcomm_dev_add() in rfcomm/tty.c is
the other caller and I have not convinced myself that the same holds
there.

-- 
Thanks,
Mikhail Gavrilov.