Patchwork slirp: Properly initialize pollfds_idx of new sockets

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 22, 2013, 7:51 p.m.
Message ID <5127CC3F.5030703@siemens.com>
Download mbox | patch
Permalink /patch/222625/
State New
Headers show

Comments

Jan Kiszka - Feb. 22, 2013, 7:51 p.m.
Otherwise we may start processing sockets in slirp_pollfds_poll that
were created past slirp_pollfds_fill.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Not sure if this pattern also applies to other users besides slirp.
Worth checking I suppose.

 slirp/socket.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Laszlo Ersek - Feb. 22, 2013, 8:17 p.m.
On 02/22/13 20:51, Jan Kiszka wrote:
> Otherwise we may start processing sockets in slirp_pollfds_poll that
> were created past slirp_pollfds_fill.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Not sure if this pattern also applies to other users besides slirp.
> Worth checking I suppose.
> 
>  slirp/socket.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index a7ab933..bb639ae 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -51,6 +51,7 @@ socreate(Slirp *slirp)
>      so->so_state = SS_NOFDREF;
>      so->s = -1;
>      so->slirp = slirp;
> +    so->pollfds_idx = -1;
>    }
>    return(so);
>  }
> 

Aaaargh. slirp_pollfds_fill() actually has three places where it does this:
- when appending a TCP socket,
- when appending a UDP socket,
- when appending an ICMP socket,
to the pollfds array.

The assumption was (I think!) that once we've iterated over all of:
- slirp->tcb
- slirp->udb
- slirp->icmp

of each "slirp_instance", then we must have covered all slirp sockets in
existence, in slirp_pollfds_fill().

I *guess* that, after we g_poll()ed, and are in slirp_pollfds_poll(), we
can create (even accept maybe?) new sockets that are put on slirp->tcb /
slirp->udb / slirp->icmp. That is, near the end of each of these control
block lists, we can encounter sockets that weren't there when we
*entered* slirp_pollfds_poll(), let alone when we called
slirp_pollfds_fill() most recently!

(It would be interesting to see an actual call chain when this happens.)

aio-posix and iohandler seem to get this right though, I believe:
- in aio_set_fd_handler(), the index is set to -1,
- qemu_iohandler_fill() does the same.

(We even might have called it "general hygiene" or some such?...)

Of course Stefan should check :)

The patch looks good to me. It surely can't hurt!

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo
Stefan Hajnoczi - Feb. 26, 2013, 9:24 a.m.
On Fri, Feb 22, 2013 at 09:17:10PM +0100, Laszlo Ersek wrote:
> On 02/22/13 20:51, Jan Kiszka wrote:
> > Otherwise we may start processing sockets in slirp_pollfds_poll that
> > were created past slirp_pollfds_fill.
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> > 
> > Not sure if this pattern also applies to other users besides slirp.
> > Worth checking I suppose.
> > 
> >  slirp/socket.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/slirp/socket.c b/slirp/socket.c
> > index a7ab933..bb639ae 100644
> > --- a/slirp/socket.c
> > +++ b/slirp/socket.c
> > @@ -51,6 +51,7 @@ socreate(Slirp *slirp)
> >      so->so_state = SS_NOFDREF;
> >      so->s = -1;
> >      so->slirp = slirp;
> > +    so->pollfds_idx = -1;
> >    }
> >    return(so);
> >  }
> > 
> 
> Aaaargh. slirp_pollfds_fill() actually has three places where it does this:
> - when appending a TCP socket,
> - when appending a UDP socket,
> - when appending an ICMP socket,
> to the pollfds array.
> 
> The assumption was (I think!) that once we've iterated over all of:
> - slirp->tcb
> - slirp->udb
> - slirp->icmp
> 
> of each "slirp_instance", then we must have covered all slirp sockets in
> existence, in slirp_pollfds_fill().
> 
> I *guess* that, after we g_poll()ed, and are in slirp_pollfds_poll(), we
> can create (even accept maybe?) new sockets that are put on slirp->tcb /
> slirp->udb / slirp->icmp. That is, near the end of each of these control
> block lists, we can encounter sockets that weren't there when we
> *entered* slirp_pollfds_poll(), let alone when we called
> slirp_pollfds_fill() most recently!
> 
> (It would be interesting to see an actual call chain when this happens.)
> 
> aio-posix and iohandler seem to get this right though, I believe:
> - in aio_set_fd_handler(), the index is set to -1,
> - qemu_iohandler_fill() does the same.
> 
> (We even might have called it "general hygiene" or some such?...)

Right, we take care to initialize pollfds_idx = -1 in iohandler.c and
aio-posix.c.

Jan: Thanks for finding and fixing this, it was an oversight on my part.
Stefan Hajnoczi - Feb. 26, 2013, 9:24 a.m.
On Fri, Feb 22, 2013 at 08:51:27PM +0100, Jan Kiszka wrote:
> Otherwise we may start processing sockets in slirp_pollfds_poll that
> were created past slirp_pollfds_fill.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Not sure if this pattern also applies to other users besides slirp.
> Worth checking I suppose.
> 
>  slirp/socket.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Anthony Liguori - Feb. 26, 2013, 10:18 p.m.
Jan Kiszka <jan.kiszka@siemens.com> writes:

> Otherwise we may start processing sockets in slirp_pollfds_poll that
> were created past slirp_pollfds_fill.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I think there's still something wrong...

Somehow, my char flow series breaks slirp.  Prior to the g_poll
conversion, this was not the case.

I'll look into it more later and post a git tree but FYI.  I suspect
moving the chardevs to GIOChannel is uncovering a latent bug in the
slirp main loop interaction.

Regards,

Anthony Liguori

> ---
>
> Not sure if this pattern also applies to other users besides slirp.
> Worth checking I suppose.
>
>  slirp/socket.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/slirp/socket.c b/slirp/socket.c
> index a7ab933..bb639ae 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -51,6 +51,7 @@ socreate(Slirp *slirp)
>      so->so_state = SS_NOFDREF;
>      so->s = -1;
>      so->slirp = slirp;
> +    so->pollfds_idx = -1;
>    }
>    return(so);
>  }
> -- 
> 1.7.3.4
Jan Kiszka - Feb. 27, 2013, 8:22 a.m.
On 2013-02-26 23:18, Anthony Liguori wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> Otherwise we may start processing sockets in slirp_pollfds_poll that
>> were created past slirp_pollfds_fill.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I think there's still something wrong...
> 
> Somehow, my char flow series breaks slirp.  Prior to the g_poll
> conversion, this was not the case.
> 
> I'll look into it more later and post a git tree but FYI.  I suspect
> moving the chardevs to GIOChannel is uncovering a latent bug in the
> slirp main loop interaction.

OK, but please pull this fix nevertheless. It is correct and makes
current git master usable again.

Thanks,
Jan

Patch

diff --git a/slirp/socket.c b/slirp/socket.c
index a7ab933..bb639ae 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -51,6 +51,7 @@  socreate(Slirp *slirp)
     so->so_state = SS_NOFDREF;
     so->s = -1;
     so->slirp = slirp;
+    so->pollfds_idx = -1;
   }
   return(so);
 }