Hi,
I've just managed to reproduce and track a few bug that so far
were escaping me. There is a race condition in the accept() code for
non-blocking L2CAP sockets, and a similar one in sendmsg. Or maybe
it's just that my code is too fast ;-)
This is the accept race :
1) L2CAP socket in non blocking mode, because program waiting
on multiple outputs.
2) Wait on socket to be readable with poll/select.
3) When socket is ready, accept() it and do what we have to do.
4) When the race occur, accept() return an error (EAGAIN).
5) We don't touch the socket and go back to poll/select.
6) Poll/select returns immediately (socket is still readable).
7) We attempt the accept(), EAGAIN, goto (5)
I didn't managed to fully identify the sendmsg race, but I
goes like this :
1) Open L2CAP socket in non blocking mode, because program
waiting on multiple outputs.
2) Connect to BT peer.
3) Wait on socket to be writeable with poll/select.
4) When socket is ready, sendmsg() and do what we have to do.
5) When the race occur, sendmsg() return an error (ENOTCONN).
...
I looked at way to fix the code, but it's not a quick fix and
there is multiple way to attack the problem. So, if one of you could
have a look at it...
Below you will find a self explanatory log of the kernel
showing the problem with accept. The first accept was successful (no
problem), the second one was racy.
Thanks in advance...
Jean
----------------------------------------------------------------------
J2 - l2cap_connect_req - parent cd451ba0 sk cd451200 state 2
J2 - bt_accept_enqueue - parent cd451ba0 backlog 0 sk cd451200 state 2
J2 - l2cap_connect_req - sk cd451200 state 6 -> 7
J2 - l2cap_config_rsp - sk cd451200 state 7 -> 1
J2 - bt_sock_poll - sk cd451ba0 sk_receive_queue 0 backlog 1 shutdown 0
J2 - l2cap_sock_accept - sk cd451ba0 timeo 0
J2 - bt_accept_dequeue - parent cd451ba0 backlog 1 newsock c8434420
J2 - bt_accept_dequeue - sk cd451200 state 1
J2 - bt_sock_poll - sk cd451200 sk_receive_queue 1 backlog 0 shutdown 0
...
J2 - l2cap_disconnect_req - sk cd451200, conn c83342e0, err 104
J2 - l2cap_chan_del - sk cd451200, conn c83342e0, err 104 state 8 -> 9
J2 - l2cap_connect_rsp - sk cd451e60 state 5 -> 7
J2 - l2cap_config_rsp - sk cd451e60 state 7 -> 1
J2 - l2cap_chan_ready - sk cd451e60 state 1 -> 1
J2 - bt_sock_poll - sk cd451e60 sk_receive_queue 1 backlog 0 shutdown 0
J2 - l2cap_disconnect_rsp - sk cd451e60, conn c83342e0, err 0
J2 - l2cap_chan_del - sk cd451e60, conn c83342e0, err 0 state 8 -> 9
----------------------------------------------------------------------
J2 - l2cap_connect_req - parent cd451ba0 sk cd451200 state 2
J2 - bt_accept_enqueue - parent cd451ba0 backlog 0 sk cd451200 state 2
J2 - l2cap_connect_req - sk cd451200 state 6 -> 7
J2 - bt_sock_poll - sk cd451ba0 sk_receive_queue 0 backlog 1 shutdown 0
J2 - l2cap_sock_accept - sk cd451ba0 timeo 0
J2 - bt_accept_dequeue - parent cd451ba0 backlog 1 newsock c84340e0
J2 - bt_accept_dequeue - sk cd451200 state 7
J2 - bt_sock_poll - sk cd451ba0 sk_receive_queue 0 backlog 1 shutdown 0
J2 - l2cap_sock_accept - sk cd451ba0 timeo 0
J2 - bt_accept_dequeue - parent cd451ba0 backlog 1 newsock c84340e0
J2 - bt_accept_dequeue - sk cd451200 state 7
J2 - bt_sock_poll - sk cd451ba0 sk_receive_queue 0 backlog 1 shutdown 0
J2 - l2cap_sock_accept - sk cd451ba0 timeo 0
J2 - bt_accept_dequeue - parent cd451ba0 backlog 1 newsock c84340e0
J2 - bt_accept_dequeue - sk cd451200 state 7
...
[[Last 3 messages repeat more or less 500 times]]
...
J2 - l2cap_sock_accept - sk cd451ba0 timeo 0
J2 - bt_accept_dequeue - parent cd451ba0 backlog 1 newsock c84340e0
J2 - bt_accept_dequeue - sk cd451200 state 7
J2 - l2cap_config_rsp - sk cd451200 state 7 -> 1
J2 - bt_sock_poll - sk cd451ba0 sk_receive_queue 0 backlog 1 shutdown 0
J2 - l2cap_sock_accept - sk cd451ba0 timeo 0
J2 - bt_accept_dequeue - parent cd451ba0 backlog 1 newsock c84340e0
J2 - bt_accept_dequeue - sk cd451200 state 1
J2 - bt_sock_poll - sk cd451200 sk_receive_queue 1 backlog 0 shutdown 0
----------------------------------------------------------------------
On Fri, Feb 06, 2004 at 12:37:24AM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > There is one thing I don't like about your patch : you disable
> > error checking for listening socket. I don't think it's right,
> > especially for checking socket shutdown.
> > Let's take the following scenario. Multi-threaded program, one
> > listening l2cap socket. One thread poll/select on the socket. At some
> > point, the second thread close the socket. I would expect the first
> > thread to receive a POLLHUP. Note that you can do the same with a
> > single threaded program, closing the listening socket in the signal
> > handler.
> > The attached patch is a minor modification of your patch that
> > should fix this issue.
>
> take a look at tcp_poll() in net/ipv4/tcp.c, because they do it the same
> and do we really have to care about it? How does TCP handles this case?
>
> Regards
>
> Marcel
Well, it never hurts to be on the safe side. And if you look
at my patch, you would see that it's only a trivial change to your
patch. Anyway, up to you...
Jean
Hi Jean,
> There is one thing I don't like about your patch : you disable
> error checking for listening socket. I don't think it's right,
> especially for checking socket shutdown.
> Let's take the following scenario. Multi-threaded program, one
> listening l2cap socket. One thread poll/select on the socket. At some
> point, the second thread close the socket. I would expect the first
> thread to receive a POLLHUP. Note that you can do the same with a
> single threaded program, closing the listening socket in the signal
> handler.
> The attached patch is a minor modification of your patch that
> should fix this issue.
take a look at tcp_poll() in net/ipv4/tcp.c, because they do it the same
and do we really have to care about it? How does TCP handles this case?
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
On Thu, Feb 05, 2004 at 07:17:37PM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > > I had the attached patch in mind. Please check if this also works for
> > > you, because it should produce less code execution. I think we can also
> > > remove the list_empty() check from bt_sock_listen_poll(), because the
> > > list_for_each_safe() gives us the same result.
> >
> > This should produce the same result. I'll try.
>
> attached is a slight modified version of my patch.
There is one thing I don't like about your patch : you disable
error checking for listening socket. I don't think it's right,
especially for checking socket shutdown.
Let's take the following scenario. Multi-threaded program, one
listening l2cap socket. One thread poll/select on the socket. At some
point, the second thread close the socket. I would expect the first
thread to receive a POLLHUP. Note that you can do the same with a
single threaded program, closing the listening socket in the signal
handler.
The attached patch is a minor modification of your patch that
should fix this issue.
> > > I already checked the IPV4 code and they also work with an accept queue,
> > > but at the moment I don't understand when they put the new socket on it
> > > and how they keep track of it.
> >
> > Actually, I will double check, because I belive that it may be
> > normal behavior. More later.
>
> The TCP code uses its own open_request structure for the accept queue,
> but I think the problem of non-blocking listen() will be the same as in
> Bluetooth.
This is "normal" behaviour. As my prog has many active sockets
and many timers, poll just get called a lot in normal operation even
is the BT socket is inactive. False alarm.
> However the fix for the sendmsg() race is correct and must be applied.
Glad you like it ;-)
> Regards
>
> Marcel
Jean
Hi Jean,
> > I had the attached patch in mind. Please check if this also works for
> > you, because it should produce less code execution. I think we can also
> > remove the list_empty() check from bt_sock_listen_poll(), because the
> > list_for_each_safe() gives us the same result.
>
> This should produce the same result. I'll try.
attached is a slight modified version of my patch.
> > I already checked the IPV4 code and they also work with an accept queue,
> > but at the moment I don't understand when they put the new socket on it
> > and how they keep track of it.
>
> Actually, I will double check, because I belive that it may be
> normal behavior. More later.
The TCP code uses its own open_request structure for the accept queue,
but I think the problem of non-blocking listen() will be the same as in
Bluetooth.
However the fix for the sendmsg() race is correct and must be applied.
Regards
Marcel
On Thu, Feb 05, 2004 at 11:46:55AM +0100, Mauro Tortonesi wrote:
>
> i think the "open a HCI socket and send HCI commands" approach (which indeed
> has many advantages) is a bit too low-level to be used in standard
> applications as a way for setting QoS over Bluez.
> since QoS is semantically an attribute of a connection, it would be nice to
> have ioctl(2) commands for setting QoS parameters on a given connection.
Aggregating the QoS of the various sockets into a generic QoS
that you can pass to the HCI is not a trivial problem. You can not
guarantee that there will only be one socket per handle.
> > Not that the main issue will be firware support. For example
> > the QoS feature I needed is not implemented in the CSR firmware.
>
> do you mean that most of the firmware of the Bluetooth chips in commerce do
> not support/implement HCI QoS commands (such as OGF 0x0002 OCF 0x0007)? what
> version of CSR firmware are you talking about?
No, I'm saying "the QoS feature I *needed*". I want QoS
*events*, most firmware don't support that.
> Mauro Tortonesi
Ciao.
Jean
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
On Thu, Feb 05, 2004 at 02:49:38PM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > So, in other words I've just moved a tight loop from my code
> > to the kernel code (or glibc). Which means it seems that we only have
> > half of the solution (or maybe it's normal poll behavior ?).
>
> I had the attached patch in mind. Please check if this also works for
> you, because it should produce less code execution. I think we can also
> remove the list_empty() check from bt_sock_listen_poll(), because the
> list_for_each_safe() gives us the same result.
This should produce the same result. I'll try.
Personally, I prefer commented code ;-)
> > I think at that point we will need to get advice from the
> > network gurus on how socket notification works.
>
> I already checked the IPV4 code and they also work with an accept queue,
> but at the moment I don't understand when they put the new socket on it
> and how they keep track of it.
Actually, I will double check, because I belive that it may be
normal behavior. More later.
Thanks !
> Regards
>
> Marcel
Jean
Hi Jean,
> So, in other words I've just moved a tight loop from my code
> to the kernel code (or glibc). Which means it seems that we only have
> half of the solution (or maybe it's normal poll behavior ?).
I had the attached patch in mind. Please check if this also works for
you, because it should produce less code execution. I think we can also
remove the list_empty() check from bt_sock_listen_poll(), because the
list_for_each_safe() gives us the same result.
> I think at that point we will need to get advice from the
> network gurus on how socket notification works.
I already checked the IPV4 code and they also work with an accept queue,
but at the moment I don't understand when they put the new socket on it
and how they keep track of it.
Regards
Marcel
On Wednesday 04 February 2004 18:46, Jean Tourrilhes wrote:
> On Wed, Feb 04, 2004 at 12:23:29PM +0100, Mauro Tortonesi wrote:
> > hi to everybody,
> >
> > i was trying to do some qos setup tests with bluez and i have seen that
> > there is barely no qos support in bluez. it would be nice to develop an
> > ioctl(2)-based extension to set up qos on bluez sockets. i have just
> > started working on the code. marcel, would you be intested in having qos
> > support for bluez?
> >
> > BTW: i have seen that Jean Tourrilhes (included in cc) has been working
> > on qos with CSR chips a while ago. jean, since i have seen no
> > announcements on this list i suppose that you haven't production-quality
> > code adding qos support for bluez...
>
> My code just open a HCI socket and send HCI commands. The HCI
> commands are pretty straightforward, well documented and do everything
> you want, so I don't see the point in adding weird ioctls. The main
> issue I have with ioctls is that they are blocking, so that's why my
> code use HCI socket for everything (including Inquiry).
i think the "open a HCI socket and send HCI commands" approach (which indeed
has many advantages) is a bit too low-level to be used in standard
applications as a way for setting QoS over Bluez.
since QoS is semantically an attribute of a connection, it would be nice to
have ioctl(2) commands for setting QoS parameters on a given connection.
> Not that the main issue will be firware support. For example
> the QoS feature I needed is not implemented in the CSR firmware.
do you mean that most of the firmware of the Bluetooth chips in commerce do
not support/implement HCI QoS commands (such as OGF 0x0002 OCF 0x0007)? what
version of CSR firmware are you talking about?
--
Aequam memento rebus in arduis servare mentem...
Mauro Tortonesi [email protected]
[email protected]
[email protected]
Deep Space 6 - IPv6 with Linux http://www.deepspace6.net
Ferrara Linux User Group http://www.ferrara.linux.it
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Jean,
> No, the code is not that simple and I don't really want it
> out. However, I think any trivial program using non-blocking sockets
> will show that.
> Anyway, I think the bug report was detailed enough.
code is always better ;) I have written some stuff by myself now.
> After around 500 iterations, it will succeed. I guess it
> depend on your CPU speed.
It seems that we signal the socket readable after the L2CAP connect
req/resp is handled. But the socket is first usable after we had a
two-way L2CAP config req/resp for MTU negotiation. And this takes some
time depending on if we have to do a role change, authenticate etc.
> Yes, listen() will not block. However I also want the accept()
> to be non-blocking, because I don't want it to prevent it servincing
> my other socekts. What's the point of writting all the rest of my code
> non-blocking if accept() take 100ms to complete ? The only way to get
> a non-blocking accept() is to have the listen socket in non-blocking
> mode.
Actually I don't know how to change this and I even don't know if this
is a good idea, because the return code of accept() seems right.
Comments?
> The sendmsg() problem is on the other side, the client, so a
> simply connected socket. But I've got a much harder time to reproduce
> it.
Haven't looked at it. We need a reliable way do reproduce it.
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
On Thu, Feb 05, 2004 at 03:36:43AM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > > What I am thinking is that
> > > if our socket is BT_LISTEN, we check for an not empty accept_q and than
> > > iterate through all sockets for BT_CONNECTED. If we found one, we return
> > > POLLIN else nothing.
> >
> > This is a bit yucky, especially that poll is performance
> > critical (for scalability). That's why I was suggesting the socket
> > counter in the parent. Check what's sk_ack_backlog does, it's very
> > close to what we want.
>
> yes I know, but worse performance only kicks in if it is a listen socket
> and if it has at minimum one child in its queue. A second counter sounds
> not really better to me and I think it will be very hackish.
>
> Regards
>
> Marcel
I implemented the option that you prefered. The patch is
attached.
I also fixed the "potential" sendmsg race as I understand it
(the one I can't reproduce).
The good news is that accept no longer returns EAGAIN and my
code no longer tight-loop.
The problem with the patch is that I get this in my log :
------------------------------------------------------------
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 1
...
...
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 0
J2 - bt_accept_checkqueue - parent ccc11600 backlog 1 num 1
------------------------------------------------------------
So, in other words I've just moved a tight loop from my code
to the kernel code (or glibc). Which means it seems that we only have
half of the solution (or maybe it's normal poll behavior ?).
I think at that point we will need to get advice from the
network gurus on how socket notification works.
Have fun...
Jean
On Thu, Feb 05, 2004 at 03:36:43AM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > > What I am thinking is that
> > > if our socket is BT_LISTEN, we check for an not empty accept_q and than
> > > iterate through all sockets for BT_CONNECTED. If we found one, we return
> > > POLLIN else nothing.
> >
> > This is a bit yucky, especially that poll is performance
> > critical (for scalability). That's why I was suggesting the socket
> > counter in the parent. Check what's sk_ack_backlog does, it's very
> > close to what we want.
>
> yes I know, but worse performance only kicks in if it is a listen socket
> and if it has at minimum one child in its queue. A second counter sounds
> not really better to me and I think it will be very hackish.
>
> Regards
>
> Marcel
Ok. I'll try my best along those lines.
Jean
Hi Jean,
> > What I am thinking is that
> > if our socket is BT_LISTEN, we check for an not empty accept_q and than
> > iterate through all sockets for BT_CONNECTED. If we found one, we return
> > POLLIN else nothing.
>
> This is a bit yucky, especially that poll is performance
> critical (for scalability). That's why I was suggesting the socket
> counter in the parent. Check what's sk_ack_backlog does, it's very
> close to what we want.
yes I know, but worse performance only kicks in if it is a listen socket
and if it has at minimum one child in its queue. A second counter sounds
not really better to me and I think it will be very hackish.
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
On Thu, Feb 05, 2004 at 03:21:46AM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> right now I don't see an easy way out of it.
I told you ;-) That's why I didn't send a patch.
> What I am thinking is that
> if our socket is BT_LISTEN, we check for an not empty accept_q and than
> iterate through all sockets for BT_CONNECTED. If we found one, we return
> POLLIN else nothing.
This is a bit yucky, especially that poll is performance
critical (for scalability). That's why I was suggesting the socket
counter in the parent. Check what's sk_ack_backlog does, it's very
close to what we want.
> Regards
>
> Marcel
Jean
Hi Jean,
> > Touching sk_ack_backlog or introducing another counter is not a good
> > idea, I think. What about moving the bt_accept_enqueue() call in the
> > L2CAP code to the right "time"?
>
> The problem with that is that you are going to loose track of
> those sockets. As accept has not been done, they are not referenced
> from user space. The only place knowing about them is parent->accept_q.
> If for some reason the config req/rsp fail, or if the user
> space doesn't accept, those sockets are going to leak. Look at the
> code in "dequeue" that reap CLOSED sockets, and also the code that
> close all childs when a socket is destroyed.
> I told you it was not totally trivial.
right now I don't see an easy way out of it. What I am thinking is that
if our socket is BT_LISTEN, we check for an not empty accept_q and than
iterate through all sockets for BT_CONNECTED. If we found one, we return
POLLIN else nothing.
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
On Thu, Feb 05, 2004 at 02:30:24AM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > > I haven't tested this yet, but I think the problem is the extra check in
> > > bt_sock_poll() for the accept_q:
> > >
> > > if (!skb_queue_empty(&sk->sk_receive_queue) ||
> > > !list_empty(&bt_sk(sk)->accept_q) ||
> > > (sk->sk_shutdown & RCV_SHUTDOWN))
> > > mask |= POLLIN | POLLRDNORM;
> >
> > Wrong. The client may only receive packet and never send
> > packets, and anyway those packets would go to the child socket, not
> > the parent socket (remember, we are before accept(), so we are polling
> > the parent/listening socket). You really need to test the *childs* of
> > this socket (i.e. those guys in accept_q).
> > One way to do this is to have a counter counting the number of
> > child sockets in CONNECTED mode but not yet dequeued. One idea is to
> > reuse sk_ack_backlog to do this.
>
> now I got it. I was thinking from the wrong direction ;)
>
> Touching sk_ack_backlog or introducing another counter is not a good
> idea, I think. What about moving the bt_accept_enqueue() call in the
> L2CAP code to the right "time"?
The problem with that is that you are going to loose track of
those sockets. As accept has not been done, they are not referenced
from user space. The only place knowing about them is parent->accept_q.
If for some reason the config req/rsp fail, or if the user
space doesn't accept, those sockets are going to leak. Look at the
code in "dequeue" that reap CLOSED sockets, and also the code that
close all childs when a socket is destroyed.
I told you it was not totally trivial.
> BTW do RFCOMM have the same problem?
Never tried.
> Regards
>
> Marcel
Jean
Hi Jean,
> > I haven't tested this yet, but I think the problem is the extra check in
> > bt_sock_poll() for the accept_q:
> >
> > if (!skb_queue_empty(&sk->sk_receive_queue) ||
> > !list_empty(&bt_sk(sk)->accept_q) ||
> > (sk->sk_shutdown & RCV_SHUTDOWN))
> > mask |= POLLIN | POLLRDNORM;
>
> Wrong. The client may only receive packet and never send
> packets, and anyway those packets would go to the child socket, not
> the parent socket (remember, we are before accept(), so we are polling
> the parent/listening socket). You really need to test the *childs* of
> this socket (i.e. those guys in accept_q).
> One way to do this is to have a counter counting the number of
> child sockets in CONNECTED mode but not yet dequeued. One idea is to
> reuse sk_ack_backlog to do this.
now I got it. I was thinking from the wrong direction ;)
Touching sk_ack_backlog or introducing another counter is not a good
idea, I think. What about moving the bt_accept_enqueue() call in the
L2CAP code to the right "time"?
BTW do RFCOMM have the same problem?
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
On Thu, Feb 05, 2004 at 02:00:18AM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > I could find an informal arrangement if you want my code, like
> > "for your eyes only", but it's also not totally trivial to setup.
>
> not needed, because it is very clear where the problem is. But thanks
> for the offer.
>
> > Yes, the accept() code is right, what's wrong is obviously the
> > "poll" code that should not indicate socket readyness before the
> > socket is really ready.
> > The more "correct" way would be to change the "poll()" code to
> > really test child socket readyness instead of just testing
> > sk_ack_backlog. But, the poll code would get very ugly quickly.
> > One of the simplest way would be to increase sk_ack_backlog
> > only when the socket change to the BT_CONNECTED state (as opposed to
> > immediately when we add it to the parent queue). I would need to
> > understand where and how exactly is sk_ack_backlog used.
> > Another solution if we can't use sk_ack_backlog is to use our
> > own counter to count the number of child ready, or a flag, or whatever
> > mechanism.
>
> I don't see what sk_ack_backlog have to do with it. Can you explain this
> to me?
See below.
> I haven't tested this yet, but I think the problem is the extra check in
> bt_sock_poll() for the accept_q:
>
> if (!skb_queue_empty(&sk->sk_receive_queue) ||
> !list_empty(&bt_sk(sk)->accept_q) ||
> (sk->sk_shutdown & RCV_SHUTDOWN))
> mask |= POLLIN | POLLRDNORM;
Wrong. The client may only receive packet and never send
packets, and anyway those packets would go to the child socket, not
the parent socket (remember, we are before accept(), so we are polling
the parent/listening socket). You really need to test the *childs* of
this socket (i.e. those guys in accept_q).
One way to do this is to have a counter counting the number of
child sockets in CONNECTED mode but not yet dequeued. One idea is to
reuse sk_ack_backlog to do this.
> Regards
>
> Marcel
Jean
Hi Jean,
> I could find an informal arrangement if you want my code, like
> "for your eyes only", but it's also not totally trivial to setup.
not needed, because it is very clear where the problem is. But thanks
for the offer.
> Yes, the accept() code is right, what's wrong is obviously the
> "poll" code that should not indicate socket readyness before the
> socket is really ready.
> The more "correct" way would be to change the "poll()" code to
> really test child socket readyness instead of just testing
> sk_ack_backlog. But, the poll code would get very ugly quickly.
> One of the simplest way would be to increase sk_ack_backlog
> only when the socket change to the BT_CONNECTED state (as opposed to
> immediately when we add it to the parent queue). I would need to
> understand where and how exactly is sk_ack_backlog used.
> Another solution if we can't use sk_ack_backlog is to use our
> own counter to count the number of child ready, or a flag, or whatever
> mechanism.
I don't see what sk_ack_backlog have to do with it. Can you explain this
to me?
I haven't tested this yet, but I think the problem is the extra check in
bt_sock_poll() for the accept_q:
if (!skb_queue_empty(&sk->sk_receive_queue) ||
!list_empty(&bt_sk(sk)->accept_q) ||
(sk->sk_shutdown & RCV_SHUTDOWN))
mask |= POLLIN | POLLRDNORM;
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
On Wed, Feb 04, 2004 at 08:58:47PM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > No, the code is not that simple and I don't really want it
> > out. However, I think any trivial program using non-blocking sockets
> > will show that.
> > Anyway, I think the bug report was detailed enough.
>
> code is always better ;) I have written some stuff by myself now.
I could find an informal arrangement if you want my code, like
"for your eyes only", but it's also not totally trivial to setup.
> > After around 500 iterations, it will succeed. I guess it
> > depend on your CPU speed.
>
> It seems that we signal the socket readable after the L2CAP connect
> req/resp is handled. But the socket is first usable after we had a
> two-way L2CAP config req/resp for MTU negotiation. And this takes some
> time depending on if we have to do a role change, authenticate etc.
Correct.
> > Yes, listen() will not block. However I also want the accept()
> > to be non-blocking, because I don't want it to prevent it servincing
> > my other socekts. What's the point of writting all the rest of my code
> > non-blocking if accept() take 100ms to complete ? The only way to get
> > a non-blocking accept() is to have the listen socket in non-blocking
> > mode.
>
> Actually I don't know how to change this and I even don't know if this
> is a good idea, because the return code of accept() seems right.
>
> Comments?
Yes, the accept() code is right, what's wrong is obviously the
"poll" code that should not indicate socket readyness before the
socket is really ready.
The more "correct" way would be to change the "poll()" code to
really test child socket readyness instead of just testing
sk_ack_backlog. But, the poll code would get very ugly quickly.
One of the simplest way would be to increase sk_ack_backlog
only when the socket change to the BT_CONNECTED state (as opposed to
immediately when we add it to the parent queue). I would need to
understand where and how exactly is sk_ack_backlog used.
Another solution if we can't use sk_ack_backlog is to use our
own counter to count the number of child ready, or a flag, or whatever
mechanism.
> > The sendmsg() problem is on the other side, the client, so a
> > simply connected socket. But I've got a much harder time to reproduce
> > it.
>
> Haven't looked at it. We need a reliable way do reproduce it.
I think the "poll()" change would fix that as well.
> Regards
>
> Marcel
Thanks...
Jean
On Wed, Feb 04, 2004 at 08:17:24AM +0100, Marcel Holtmann wrote:
> Hi Jean,
>
> > I've just managed to reproduce and track a few bug that so far
> > were escaping me. There is a race condition in the accept() code for
> > non-blocking L2CAP sockets, and a similar one in sendmsg. Or maybe
> > it's just that my code is too fast ;-)
>
> do you have a simple test code to reproduce it?
No, the code is not that simple and I don't really want it
out. However, I think any trivial program using non-blocking sockets
will show that.
Anyway, I think the bug report was detailed enough.
> > This is the accept race :
> > 1) L2CAP socket in non blocking mode, because program waiting
> > on multiple outputs.
> > 2) Wait on socket to be readable with poll/select.
> > 3) When socket is ready, accept() it and do what we have to do.
> > 4) When the race occur, accept() return an error (EAGAIN).
> > 5) We don't touch the socket and go back to poll/select.
> > 6) Poll/select returns immediately (socket is still readable).
> > 7) We attempt the accept(), EAGAIN, goto (5)
>
> Is this an endless loop? Or do accept() succeeds after some time?
After around 500 iterations, it will succeed. I guess it
depend on your CPU speed.
> BTW why must a listen socket non-blocking? The listen() command itself
> doesn't block and I don't see any need for a non-blocking listen socket.
Yes, listen() will not block. However I also want the accept()
to be non-blocking, because I don't want it to prevent it servincing
my other socekts. What's the point of writting all the rest of my code
non-blocking if accept() take 100ms to complete ? The only way to get
a non-blocking accept() is to have the listen socket in non-blocking
mode.
> > I didn't managed to fully identify the sendmsg race, but I
> > goes like this :
> > 1) Open L2CAP socket in non blocking mode, because program
> > waiting on multiple outputs.
> > 2) Connect to BT peer.
> > 3) Wait on socket to be writeable with poll/select.
> > 4) When socket is ready, sendmsg() and do what we have to do.
> > 5) When the race occur, sendmsg() return an error (ENOTCONN).
> > ...
> >
> > I looked at way to fix the code, but it's not a quick fix and
> > there is multiple way to attack the problem. So, if one of you could
> > have a look at it...
>
> Is this on a listen/accepted socket or is this a simple connection?
The sendmsg() problem is on the other side, the client, so a
simply connected socket. But I've got a much harder time to reproduce
it.
> > Below you will find a self explanatory log of the kernel
> > showing the problem with accept. The first accept was successful (no
> > problem), the second one was racy.
>
> >From the logfile function names I assume this is a 2.6 kernel. Do you
> see the same behaviour on 2.4?
I saw the behaviour initially in 2.4.21, reproduced it on
2.4.23-rc2, investigated with 2.6.2-rc1, and verified that the code
was functionally similar in 2.4.23-rc2.
> Regards
>
> Marcel
Thanks...
Jean
On Wed, Feb 04, 2004 at 12:23:29PM +0100, Mauro Tortonesi wrote:
>
> hi to everybody,
>
> i was trying to do some qos setup tests with bluez and i have seen that there
> is barely no qos support in bluez. it would be nice to develop an
> ioctl(2)-based extension to set up qos on bluez sockets. i have just started
> working on the code. marcel, would you be intested in having qos support for
> bluez?
>
> BTW: i have seen that Jean Tourrilhes (included in cc) has been working on qos
> with CSR chips a while ago. jean, since i have seen no announcements on this
> list i suppose that you haven't production-quality code adding qos support
> for bluez...
My code just open a HCI socket and send HCI commands. The HCI
commands are pretty straightforward, well documented and do everything
you want, so I don't see the point in adding weird ioctls. The main
issue I have with ioctls is that they are blocking, so that's why my
code use HCI socket for everything (including Inquiry).
Not that the main issue will be firware support. For example
the QoS feature I needed is not implemented in the CSR firmware.
Ciao...
Jean
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Mauro,
> i was trying to do some qos setup tests with bluez and i have seen that there
> is barely no qos support in bluez. it would be nice to develop an
> ioctl(2)-based extension to set up qos on bluez sockets. i have just started
> working on the code. marcel, would you be intested in having qos support for
> bluez?
I never used any QoS feature of Bluetooth so far, but go ahead and
implement it. Please start working with a 2.6 kernel and don't worry
about showing us early and experimental code.
> BTW: i have seen that Jean Tourrilhes (included in cc) has been working on qos
> with CSR chips a while ago. jean, since i have seen no announcements on this
> list i suppose that you haven't production-quality code adding qos support
> for bluez...
If my memory works right, there was a problem with older CSR firmwares
and QoS on the HCI level.
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
hi to everybody,
i was trying to do some qos setup tests with bluez and i have seen that there
is barely no qos support in bluez. it would be nice to develop an
ioctl(2)-based extension to set up qos on bluez sockets. i have just started
working on the code. marcel, would you be intested in having qos support for
bluez?
BTW: i have seen that Jean Tourrilhes (included in cc) has been working on qos
with CSR chips a while ago. jean, since i have seen no announcements on this
list i suppose that you haven't production-quality code adding qos support
for bluez...
--
Aequam memento rebus in arduis servare mentem...
Mauro Tortonesi [email protected]
[email protected]
[email protected]
Deep Space 6 - IPv6 with Linux http://www.deepspace6.net
Ferrara Linux User Group http://www.ferrara.linux.it
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Jean,
> I've just managed to reproduce and track a few bug that so far
> were escaping me. There is a race condition in the accept() code for
> non-blocking L2CAP sockets, and a similar one in sendmsg. Or maybe
> it's just that my code is too fast ;-)
do you have a simple test code to reproduce it?
> This is the accept race :
> 1) L2CAP socket in non blocking mode, because program waiting
> on multiple outputs.
> 2) Wait on socket to be readable with poll/select.
> 3) When socket is ready, accept() it and do what we have to do.
> 4) When the race occur, accept() return an error (EAGAIN).
> 5) We don't touch the socket and go back to poll/select.
> 6) Poll/select returns immediately (socket is still readable).
> 7) We attempt the accept(), EAGAIN, goto (5)
Is this an endless loop? Or do accept() succeeds after some time?
BTW why must a listen socket non-blocking? The listen() command itself
doesn't block and I don't see any need for a non-blocking listen socket.
> I didn't managed to fully identify the sendmsg race, but I
> goes like this :
> 1) Open L2CAP socket in non blocking mode, because program
> waiting on multiple outputs.
> 2) Connect to BT peer.
> 3) Wait on socket to be writeable with poll/select.
> 4) When socket is ready, sendmsg() and do what we have to do.
> 5) When the race occur, sendmsg() return an error (ENOTCONN).
> ...
>
> I looked at way to fix the code, but it's not a quick fix and
> there is multiple way to attack the problem. So, if one of you could
> have a look at it...
Is this on a listen/accepted socket or is this a simple connection?
> Below you will find a self explanatory log of the kernel
> showing the problem with accept. The first accept was successful (no
> problem), the second one was racy.
>>From the logfile function names I assume this is a 2.6 kernel. Do you
see the same behaviour on 2.4?
Regards
Marcel
-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel