linux-kernel - Re: [PATCH v2] Bluetooth: keep LE flow credits when recvbuf full

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <73b930f2-02b9-42cc-9e77-aecfa6601387@surban.net>
Date: Sun, 7 Apr 2024 18:57:07 +0000
From: Sebastian Urban <surban@...ban.net>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
CC: Marcel Holtmann <marcel@...tmann.org>, Johan Hedberg
	<johan.hedberg@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, "linux-bluetooth@...r.kernel.org"
	<linux-bluetooth@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] Bluetooth: keep LE flow credits when recvbuf full

Hi Luiz,

On 4/5/24 17:30, Luiz Augusto von Dentz wrote:
> Hi Sebastian,
> 
> On Fri, Apr 5, 2024 at 6:26 AM Sebastian Urban <surban@...ban.net> wrote:
>>
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -682,10 +682,15 @@ struct l2cap_user {
>>   /* ----- L2CAP socket info ----- */
>>   #define l2cap_pi(sk) ((struct l2cap_pinfo *) sk)
>>
>> +struct l2cap_rx_busy {
>> +       struct list_head        list;
>> +       struct sk_buff          *skb;
>> +};
> 
> In theory we only really to queue 1 skb at most, since we would stop
> giving credits, or perhaps this is because we had given enough credits
> for MTU + 1, so the +1 segment could result in a second SDU/skb to be
> completed while waiting the user space process to start reading again?

Yes, during testing it became apparent that there might be a second 
incoming skb, which also needs to be buffered.

Even if --as discussed below-- we change send_credits to return credits 
based on the actual available receive buffer space, I believe we still 
need to allow buffering more than one skb. This is because local 
user-space might decide to resize the receive buffer size (SO_RCVBUF) to 
a smaller value after the credits have already been given to the remote 
side.

>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index ab5a9d42fae7..c78af7fad255 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -63,6 +63,8 @@ static void l2cap_retrans_timeout(struct work_struct *work);
>>   static void l2cap_monitor_timeout(struct work_struct *work);
>>   static void l2cap_ack_timeout(struct work_struct *work);
>>
>> +static void l2cap_chan_le_send_credits(struct l2cap_chan *chan);
> 
> We probably need to change the way send_credits calculates the number
> of credits to be restored, it needs to consider the actual available
> buffer size at the socket rather then assuming we always shall have
> space for MTU + 1, that way the remote side would always have the
> exact information of how much buffer space is left. That said perhaps
> we need a way to inform when user space reads then we need to call
> into send_credits again.

Yes, this makes sense. I will extend the patch appropriately.

>> @@ -1187,11 +1189,15 @@ static int l2cap_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>>          if (!test_bit(CONN_LOCAL_BUSY, &pi->chan->conn_state))
>>                  goto done;
>>
>> -       if (pi->rx_busy_skb) {
>> -               if (!__sock_queue_rcv_skb(sk, pi->rx_busy_skb))
>> -                       pi->rx_busy_skb = NULL;
>> -               else
>> +       while (!list_empty(&pi->rx_busy)) {
>> +               struct l2cap_rx_busy *rx_busy =
>> +                       list_first_entry(&pi->rx_busy,
>> +                                        struct l2cap_rx_busy,
>> +                                        list);
>> +               if (__sock_queue_rcv_skb(sk, rx_busy->skb) < 0)
>>                          goto done;
>> +               list_del(&rx_busy->list);
>> +               kfree(rx_busy);
> 
> I see now, this is trying to dequeue packets if the socket is read,
> which in case we turn the send_credits function to calculate the
> credits based on the socket buffer size that would not be necessary
> but then we would need to call into send_credits here.

This is followed by (unmodified):

	if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf >> 1)
		l2cap_chan_busy(pi->chan, 0);

And will in fact call send_credits through l2cap_chan_busy from here 
once all queued skbs have been accepted by the socket and its receive 
buffer has become half empty.