netdev - Re: [PATCH v5] Bluetooth: compute LE flow credits based on recvbuf space

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] [day] [month] [year] [list]

Message-ID: <ca4bb4bf-fe80-46ba-bb4e-4b9cb6b5b570@surban.net>
Date: Tue, 30 Apr 2024 23:51:16 +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 v5] Bluetooth: compute LE flow credits based on recvbuf
 space

Hi Luiz,

thanks for the review!

On 4/10/24 16:38, Luiz Augusto von Dentz wrote:
>> ---
>>   include/net/bluetooth/l2cap.h | 10 +++++-
>>   net/bluetooth/l2cap_core.c    | 51 ++++++++++++++++++++++----
>>   net/bluetooth/l2cap_sock.c    | 67 +++++++++++++++++++++++++----------
>>   3 files changed, 103 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 3f4057ced971..bc6ff40ebc9f 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -547,6 +547,8 @@ struct l2cap_chan {
>>
>>          __u16           tx_credits;
>>          __u16           rx_credits;
>> +       int             rx_avail;
>> +       int             rx_staged;
> 
> I'd probably use size_t for these above, and put some comments of what
> they refer to e.g. rx_staged is what has been received already, right?
> Couldn't we use chan->sdu->len instead?

Changed and replaced by chan->sdu->len.

>> @@ -535,6 +538,26 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan)
>>   }
>>   EXPORT_SYMBOL_GPL(l2cap_chan_set_defaults);
>>
>> +static __u16 l2cap_le_rx_credits(struct l2cap_chan *chan)
>> +{
>> +       if (chan->mps == 0)
>> +               return 0;
>> +
>> +       /* If we don't know the available space in the receiver buffer, give
>> +        * enough credits for a full packet.
>> +        */
>> +       if (chan->rx_avail == -1)
>> +               return (chan->imtu / chan->mps) + 1;
>> +
>> +       /* If we know how much space is available in the receive buffer, give
>> +        * out as many credits as would fill the buffer.
>> +        */
>> +       if (chan->rx_avail <= chan->rx_staged)
>> +               return 0;
> 
> Missing space.

Done.

> 
>> +       return min_t(int, U16_MAX,
>> +                    (chan->rx_avail - chan->rx_staged) / chan->mps);
> 
> We probably need to use DIV_ROUND_UP since the division can return 0
> or is that intentional since that means we cannot store another full
> mps? I think I'd give it a credit since this may impact the throughput
> if we are not given credits when just a few bytes would be necessary
> and in any case we would store the extra bytes in the rx_busy list if
> that is over the rx_avail.

I agree. Changed.

>> @@ -6541,6 +6561,16 @@ static void l2cap_chan_le_send_credits(struct l2cap_chan *chan)
>>          l2cap_send_cmd(conn, chan->ident, L2CAP_LE_CREDITS, sizeof(pkt), &pkt);
>>   }
>>
>> +void l2cap_chan_rx_avail(struct l2cap_chan *chan, int rx_avail)
>> +{
>> +       BT_DBG("chan %p has %d bytes avail for rx", chan, rx_avail);
> 
> We should probably check if the value has changed though, or this
> shall only be called when the buffer changes?

Function returns now immediately if rx_avail is unchanged.

Additionally I improved the available receive space estimation by taking 
the overhead of struct sk_buff into account.

I will submit a new version of the patch soon.