[ECOS] uipc_socket.c

[ECOS] uipc_socket.c

[Will Lentz]

Hi,

I may have found a potential bug in
packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be
completely wrong :-).

At the end of sodealloc(), the following code exists:
       zfreei(so->so_zone, so);
       wakeup(so->so_zone);
The problem is that zfreei() changes so->so_zone.  Shouldn't wakeup() be
done on the original so->so_zone?  I only noticed this problem by:
1- while(1) { 
   sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP );
   connect( sock, ... );
   close( sock );
   }
   Eventually this pauses in socket() (in cyg_tsleep()) when you run out
of eCos sockets.  

2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0.  Why?
The zfreei() call in sodealloc() changes so->so_zone to 0 before the
wakeup() call.

The following diff solves the problem for me by making the wakeup() work
on the so_zone that was freed:

--- uipc_socket.c       Thu Jul 24 11:04:25 2003
+++ new_uipc_socket.c   Wed Jun 15 14:54:20 2005
@@ -202,8 +202,12 @@
                FREE(so->so_accf, M_ACCF);
        }
 #endif /* INET */
-       zfreei(so->so_zone, so);
-        wakeup(so->so_zone);
+       {
+               struct vm_zone *tmp = so->so_zone;
+
+               zfreei(so->so_zone, so);
+               wakeup(tmp);
+       }
 }

Any ideas?  Suggestions?

Thanks,
Will


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Hans Hübner]

On Wed, 15 Jun 2005, Will Lentz wrote:

> I may have found a potential bug in
> packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be
> completely wrong :-).
>
> At the end of sodealloc(), the following code exists:
>       zfreei(so->so_zone, so);
>       wakeup(so->so_zone);
> The problem is that zfreei() changes so->so_zone.  Shouldn't wakeup() be
> done on the original so->so_zone?  I only noticed this problem by:
> 1- while(1) {
>   sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP );
>   connect( sock, ... );
>   close( sock );
>   }
>   Eventually this pauses in socket() (in cyg_tsleep()) when you run out
> of eCos sockets.
>
> 2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0.  Why?
> The zfreei() call in sodealloc() changes so->so_zone to 0 before the
> wakeup() call.

If I read the source correctly, zfreei() is a function, not a macro, and it 
is passed the zone argument by value (as is default with C).  Thus, zfreei() 
cannot change the so structure itself and the value of so->so_zone will be 
unmodified after the call.  sleep() and wakeup() take void* as argument and do 
not interpret the value, so to them it is not of concern whether the passed 
pointer is actually valid.

I looked into this because I had some problems with eCos and an embedded web 
server.  Seemingly, the server worked fine but stopped serving after some 10 
requests had been processed.  Looking into this, I found that the number of 
sockets in our default configuration was very low (16).  As TCP sockets stay 
allocated after the connection has closed until the tcp_maxidle period has 
expired, no new connections could be accepted until that period (which is in 
the minutes range in the default configuration) passed.

Not having looked at the TCP specification, I decided that reducing the 
tcp_maxidle parameter to a very low value combined with an increase in the 
number of sockets would help me out:

   extern int cyg_tcp_maxidle;
   cyg_tcp_maxidle = 200;

I am still not completely satisfied with the solution, because I don't really 
know whether this change will result in bad interactions with other TCP 
network partners.  Also, I have found the numbers reported for TCP 
accept/established/close do not correspond and I suspect that there are in 
fact other bugs which get triggered by hitting the socket limit very often.

I hope this is useful to some.

Regards,
Hans

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Will Lentz]

Hi Hans,

zfreei() is a function, and doesn't directly modify its first argument.
But - it passes "so" as its second argument, and it modifies the value
of so->so_zone through that.  Looking at zfreei:
{
    elem *p = (elem *)item;  // item here is "so"

    log(LOG_MDEBUG, "zfreei to %s <= %p\n", zone->name, p);
    p->next = zone->pool;  // p->next corresponds to so->so_zone in
memory
    zone->pool = p;
    zone->free++;
    zone->alloc_frees++;
}

It's true that wakeup() doesn't care if it is passed a valid argument,
but if it gets the wrong value then it doesn't wake up the correct
people :-).

Maybe zfreei() shouldn't be passed "so" as its second argument in
sodealloc?  Can anyone comment on why this is done?  It looks fishy
casting a "so" pointer to an "elem *" and then mucking with it...  Every
other example of zfreei() I can find passes the value returned from
zalloci() as the second argument of zfreei().

Thanks,
Will

P.S. I also found that changing cyg_tcp_maxidle helps, but I still think
there is some problem.

On Thu, 2005-06-16 at 09:01 +0200, Hans Hübner wrote:
> On Wed, 15 Jun 2005, Will Lentz wrote:
> 
> > I may have found a potential bug in
> > packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be
> > completely wrong :-).
> >
> > At the end of sodealloc(), the following code exists:
> >       zfreei(so->so_zone, so);
> >       wakeup(so->so_zone);
> > The problem is that zfreei() changes so->so_zone.  Shouldn't wakeup() be
> > done on the original so->so_zone?  I only noticed this problem by:
> > 1- while(1) {
> >   sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP );
> >   connect( sock, ... );
> >   close( sock );
> >   }
> >   Eventually this pauses in socket() (in cyg_tsleep()) when you run out
> > of eCos sockets.
> >
> > 2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0.  Why?
> > The zfreei() call in sodealloc() changes so->so_zone to 0 before the
> > wakeup() call.
> 
> If I read the source correctly, zfreei() is a function, not a macro, and it 
> is passed the zone argument by value (as is default with C).  Thus, zfreei() 
> cannot change the so structure itself and the value of so->so_zone will be 
> unmodified after the call.  sleep() and wakeup() take void* as argument and do 
> not interpret the value, so to them it is not of concern whether the passed 
> pointer is actually valid.
> 
> I looked into this because I had some problems with eCos and an embedded web 
> server.  Seemingly, the server worked fine but stopped serving after some 10 
> requests had been processed.  Looking into this, I found that the number of 
> sockets in our default configuration was very low (16).  As TCP sockets stay 
> allocated after the connection has closed until the tcp_maxidle period has 
> expired, no new connections could be accepted until that period (which is in 
> the minutes range in the default configuration) passed.
> 
> Not having looked at the TCP specification, I decided that reducing the 
> tcp_maxidle parameter to a very low value combined with an increase in the 
> number of sockets would help me out:
> 
>    extern int cyg_tcp_maxidle;
>    cyg_tcp_maxidle = 200;
> 
> I am still not completely satisfied with the solution, because I don't really 
> know whether this change will result in bad interactions with other TCP 
> network partners.  Also, I have found the numbers reported for TCP 
> accept/established/close do not correspond and I suspect that there are in 
> fact other bugs which get triggered by hitting the socket limit very often.
> 
> I hope this is useful to some.
> 
> Regards,
> Hans

--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Hans Hübner]

On Thu, 16 Jun 2005, Will Lentz wrote:

> zfreei() is a function, and doesn't directly modify its first argument.
> But - it passes "so" as its second argument, and it modifies the value
> of so->so_zone through that.  Looking at zfreei:

Oops, it helps being awake while posting!  Thanks for the patch, I'll apply it 
to my source tree immediately :)

Cheers,
Hans

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Will Lentz]

The patch does help me when I run out of sockets via the socket() call,
but I'm wondering if the zfreei() in sodealloc() is getting the correct
second argument.  I'll dig into this deeper...

Cheers,
Will

On Thu, 2005-06-16 at 17:46 +0200, Hans Hübner wrote:
> On Thu, 16 Jun 2005, Will Lentz wrote:
> 
> > zfreei() is a function, and doesn't directly modify its first argument.
> > But - it passes "so" as its second argument, and it modifies the value
> > of so->so_zone through that.  Looking at zfreei:
> 
> Oops, it helps being awake while posting!  Thanks for the patch, I'll apply it 
> to my source tree immediately :)
> 
> Cheers,
> Hans

--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Will Lentz]

Hi Hans,

No problem :-).  I'm forwarding this on to the list so others know it
helps.

BTW - any comments from people?  In uipc_socket.c soalloc() the
zalloci() is assigned to "so", so it looks right to do the zfreei() on
"so".

Can my patch be applied to CVS?

Thanks,
Will

On Thu, 2005-06-16 at 18:40 +0200, Hans Hübner wrote:
> Hi Will,
> 
> thanks for the patch, really!  I only started with eCos 2 weeks ago and I 
> stumbled across this very problem soon, as the default number of sockets is so 
> low and the bug seems to be triggered by socket shortage.  With the patch 
> applied, my application seems to run much more stable and also the TCP 
> counters now look plausible.  You really made my day, thanks!
> 
> -Hans
> 
> On Thu, 16 Jun 2005, Will Lentz wrote:
> 
> > The patch does help me when I run out of sockets via the socket() call,
> > but I'm wondering if the zfreei() in sodealloc() is getting the correct
> > second argument.  I'll dig into this deeper...
> >
> > Cheers,
> > Will
> >
> > On Thu, 2005-06-16 at 17:46 +0200, Hans Hübner wrote:
> >> On Thu, 16 Jun 2005, Will Lentz wrote:
> >>
> >>> zfreei() is a function, and doesn't directly modify its first argument.
> >>> But - it passes "so" as its second argument, and it modifies the value
> >>> of so->so_zone through that.  Looking at zfreei:
> >>
> >> Oops, it helps being awake while posting!  Thanks for the patch, I'll apply it
> >> to my source tree immediately :)
> >>
> >> Cheers,
> >> Hans
> >
> >
> >

--
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Andrew Lunn]

On Thu, Jun 16, 2005 at 04:45:23PM -0700, Will Lentz wrote:
> Hi Hans,
> 
> No problem :-).  I'm forwarding this on to the list so others know it
> helps.
> 
> BTW - any comments from people?  In uipc_socket.c soalloc() the
> zalloci() is assigned to "so", so it looks right to do the zfreei() on
> "so".
 
There has been problems like this in the past because of the missing
userspace/kernel space transition which FreeBSD has but eCos does not
have. Im out on a customer visit at the moment so don't have the
oportunity to look at this now, but i will take a look once im back.
This things are normally very easy to fix if its what i think it is.

        Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Andrew Lunn]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Thu, Jun 16, 2005 at 09:01:23AM +0200, Hans H?bner wrote:
> On Wed, 15 Jun 2005, Will Lentz wrote:
> 
> >I may have found a potential bug in
> >packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be
> >completely wrong :-).
> >
> >At the end of sodealloc(), the following code exists:
> >      zfreei(so->so_zone, so);
> >      wakeup(so->so_zone);
> >The problem is that zfreei() changes so->so_zone.  Shouldn't wakeup() be
> >done on the original so->so_zone?  I only noticed this problem by:
> >1- while(1) {
> >  sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP );
> >  connect( sock, ... );
> >  close( sock );
> >  }
> >  Eventually this pauses in socket() (in cyg_tsleep()) when you run out
> >of eCos sockets.
> >
> >2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0.  Why?
> >The zfreei() call in sodealloc() changes so->so_zone to 0 before the
> >wakeup() call.

This is not quite correct. zfreei() does not change so->so_zone. What
it does is return the memory for the so structure to the pool. The
wakeup then uses the memory which has just been returned to the
pool. There is a race condition. Once back into the pool the memory
could be allocated to another thread before the call to wakeup is
made.

Attached is a patch to fix this.

        Andrew

[-- Attachment #2: sodealloc.diff --]
[-- Type: text/plain, Size: 1495 bytes --]

Index: net/bsd_tcpip/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/bsd_tcpip/current/ChangeLog,v
retrieving revision 1.52
diff -u -r1.52 ChangeLog
--- net/bsd_tcpip/current/ChangeLog	27 Mar 2005 18:18:13 -0000	1.52
+++ net/bsd_tcpip/current/ChangeLog	17 Jun 2005 20:08:29 -0000
@@ -1,3 +1,8 @@
+2005-06-17  Andrew Lunn  <andrew.lunn@ascom.ch>
+
+	* src/sys/kern/uipc_socket.c (sodealloc): Fixed a race condition
+	when freeing the socket memory. Problem reported by Will Lent.
+
 2005-03-27  Andrew Lunn  <andrew.lunn@ascom.ch>
 
 	* src/sys/net/if.c (ifioctl): Fixed a compiler warning about 
Index: net/bsd_tcpip/current/src/sys/kern/uipc_socket.c
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c,v
retrieving revision 1.3
diff -u -r1.3 uipc_socket.c
--- net/bsd_tcpip/current/src/sys/kern/uipc_socket.c	24 Jul 2003 18:04:25 -0000	1.3
+++ net/bsd_tcpip/current/src/sys/kern/uipc_socket.c	17 Jun 2005 20:08:31 -0000
@@ -188,8 +188,10 @@
 void
 sodealloc(so)
 	struct socket *so;
+        
 {
-
+        vm_zone_t zone;
+  
 	so->so_gencnt = ++so_gencnt;
 #ifdef INET
 	if (so->so_accf != NULL) {
@@ -202,8 +204,9 @@
 		FREE(so->so_accf, M_ACCF);
 	}
 #endif /* INET */
-	zfreei(so->so_zone, so);
-        wakeup(so->so_zone);
+        zone = so->so_zone;
+        zfreei(zone, so);
+        wakeup(zone);
 }
 
 int


[-- Attachment #3: Type: text/plain, Size: 148 bytes --]

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] uipc_socket.c (and cyg_tcp_maxidle)

[Will Lentz]

Thanks!  That works great :-)

On Fri, 2005-06-17 at 22:09 +0200, Andrew Lunn wrote: 
> On Thu, Jun 16, 2005 at 09:01:23AM +0200, Hans H?bner wrote:
> > On Wed, 15 Jun 2005, Will Lentz wrote:
> > 
> > >I may have found a potential bug in
> > >packages/net/bsd_tcpip/current/src/sys/kern/uipc_socket.c (or I may be
> > >completely wrong :-).
> > >
> > >At the end of sodealloc(), the following code exists:
> > >      zfreei(so->so_zone, so);
> > >      wakeup(so->so_zone);
> > >The problem is that zfreei() changes so->so_zone.  Shouldn't wakeup() be
> > >done on the original so->so_zone?  I only noticed this problem by:
> > >1- while(1) {
> > >  sock = socket( AF_INET, SOCK_STREAM, IPPROTO_TCP );
> > >  connect( sock, ... );
> > >  close( sock );
> > >  }
> > >  Eventually this pauses in socket() (in cyg_tsleep()) when you run out
> > >of eCos sockets.
> > >
> > >2- After 2*MSL or so, cyg_wakeup() gets called with chan == 0x0.  Why?
> > >The zfreei() call in sodealloc() changes so->so_zone to 0 before the
> > >wakeup() call.
> 
> This is not quite correct. zfreei() does not change so->so_zone. What
> it does is return the memory for the so structure to the pool. The
> wakeup then uses the memory which has just been returned to the
> pool. There is a race condition. Once back into the pool the memory
> could be allocated to another thread before the call to wakeup is
> made.
> 
> Attached is a patch to fix this.
> 
>         Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss