| Message ID | 1360926013-23725-1-git-send-email-vitaly.chipounov@epfl.ch |
|---|---|
| State | New |
| Headers | show |
On 2013-02-15 12:00, Vitaly Chipounov wrote: > A socket may still have references to it in various queues > at the time it is freed, causing memory corruptions. Did you see it in practice? Or is this patch based on code review? What will happen if those queued mbufs find their ifq_so NULL? > > Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch> > --- > slirp/socket.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/slirp/socket.c b/slirp/socket.c > index 77b0c98..8a7adc8 100644 > --- a/slirp/socket.c > +++ b/slirp/socket.c > @@ -55,6 +55,33 @@ socreate(Slirp *slirp) > return(so); > } > > +/** > + * It may happen that a socket is still referenced in various > + * mbufs at the time it is freed. Clear all references to the > + * socket here. > + */ > +static void soremovefromqueues(struct socket *so) > +{ > + if (!so->so_queued) { > + return; > + } > + > + Slirp *slirp = so->slirp; > + struct mbuf *ifm; > + > + for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) { This line and the one below smells like checkpatch.pl wasn't run against it. Granted, slirp patches easy make checkpatch upset as the input is not properly formatted, but please don't add more violations. > + if (ifm->ifq_so == so) { > + ifm->ifq_so = NULL; > + } > + } > + > + for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) { > + if (ifm->ifq_so == so) { > + ifm->ifq_so = NULL; > + } > + } > +} > + > /* > * remque and free a socket, clobber cache > */ > @@ -79,6 +106,8 @@ sofree(struct socket *so) > if(so->so_next && so->so_prev) > remque(so); /* crashes if so is not in a queue */ > > + soremovefromqueues(so); > + > free(so); > } > > Sorry for the late feedback, Jan
On Fri, Feb 15, 2013 at 12:00:13PM +0100, Vitaly Chipounov wrote: > A socket may still have references to it in various queues > at the time it is freed, causing memory corruptions. > > Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch> > --- > slirp/socket.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/slirp/socket.c b/slirp/socket.c > index 77b0c98..8a7adc8 100644 > --- a/slirp/socket.c > +++ b/slirp/socket.c > @@ -55,6 +55,33 @@ socreate(Slirp *slirp) > return(so); > } > > +/** > + * It may happen that a socket is still referenced in various > + * mbufs at the time it is freed. Clear all references to the > + * socket here. > + */ > +static void soremovefromqueues(struct socket *so) > +{ > + if (!so->so_queued) { > + return; > + } > + > + Slirp *slirp = so->slirp; > + struct mbuf *ifm; Declarations should come at the beginning of a block/function (see: ... er, I could've sworn it was in HACKING or CODING_STYLE but maybe not. That's the standard in any case) > + > + for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) { > + if (ifm->ifq_so == so) { > + ifm->ifq_so = NULL; > + } > + } > + > + for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) { > + if (ifm->ifq_so == so) { > + ifm->ifq_so = NULL; > + } > + } Is the intention just to mark them null or actually remove? Not sure which, but either the logic should change or the function name should (soinvalidate() or something along that line if the former). > +} > + > /* > * remque and free a socket, clobber cache > */ > @@ -79,6 +106,8 @@ sofree(struct socket *so) > if(so->so_next && so->so_prev) > remque(so); /* crashes if so is not in a queue */ > > + soremovefromqueues(so); > + > free(so); > } > > -- > 1.7.10 > >
On Thu, Feb 21, 2013 at 03:47:25PM -0600, mdroth wrote: > On Fri, Feb 15, 2013 at 12:00:13PM +0100, Vitaly Chipounov wrote: > > A socket may still have references to it in various queues > > at the time it is freed, causing memory corruptions. > > > > Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch> Meant to cc qemu-stable > > --- > > slirp/socket.c | 29 +++++++++++++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/slirp/socket.c b/slirp/socket.c > > index 77b0c98..8a7adc8 100644 > > --- a/slirp/socket.c > > +++ b/slirp/socket.c > > @@ -55,6 +55,33 @@ socreate(Slirp *slirp) > > return(so); > > } > > > > +/** > > + * It may happen that a socket is still referenced in various > > + * mbufs at the time it is freed. Clear all references to the > > + * socket here. > > + */ > > +static void soremovefromqueues(struct socket *so) > > +{ > > + if (!so->so_queued) { > > + return; > > + } > > + > > + Slirp *slirp = so->slirp; > > + struct mbuf *ifm; > > Declarations should come at the beginning of a block/function (see: ... > er, I could've sworn it was in HACKING or CODING_STYLE but maybe not. > That's the standard in any case) > > > + > > + for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) { > > + if (ifm->ifq_so == so) { > > + ifm->ifq_so = NULL; > > + } > > + } > > + > > + for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) { > > + if (ifm->ifq_so == so) { > > + ifm->ifq_so = NULL; > > + } > > + } > > Is the intention just to mark them null or actually remove? Not sure > which, but either the logic should change or the function name should > (soinvalidate() or something along that line if the former). > > > +} > > + > > /* > > * remque and free a socket, clobber cache > > */ > > @@ -79,6 +106,8 @@ sofree(struct socket *so) > > if(so->so_next && so->so_prev) > > remque(so); /* crashes if so is not in a queue */ > > > > + soremovefromqueues(so); > > + > > free(so); > > } > > > > -- > > 1.7.10 > > > >
Hi, On 21.02.2013 15:33, Jan Kiszka wrote: > On 2013-02-15 12:00, Vitaly Chipounov wrote: >> A socket may still have references to it in various queues >> at the time it is freed, causing memory corruptions. > Did you see it in practice? Or is this patch based on code review? What > will happen if those queued mbufs find their ifq_so NULL? I have a packet trace that triggers this problem when it is injected into the guest's NIC. I am still not quite sure why this happens, but suspect that it could be caused by malformed/partial TCP/IP streams. Setting ifq_so to NULL shouldn't be an issue because there seems to be checks for that (e.g., in if.c). It doesn't seem to affect normal web browsing/downloading in the guest. Vitaly > >> Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch> >> --- >> slirp/socket.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/slirp/socket.c b/slirp/socket.c >> index 77b0c98..8a7adc8 100644 >> --- a/slirp/socket.c >> +++ b/slirp/socket.c >> @@ -55,6 +55,33 @@ socreate(Slirp *slirp) >> return(so); >> } >> >> +/** >> + * It may happen that a socket is still referenced in various >> + * mbufs at the time it is freed. Clear all references to the >> + * socket here. >> + */ >> +static void soremovefromqueues(struct socket *so) >> +{ >> + if (!so->so_queued) { >> + return; >> + } >> + >> + Slirp *slirp = so->slirp; >> + struct mbuf *ifm; >> + >> + for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) { > This line and the one below smells like checkpatch.pl wasn't run against > it. Granted, slirp patches easy make checkpatch upset as the input is > not properly formatted, but please don't add more violations. > >> + if (ifm->ifq_so == so) { >> + ifm->ifq_so = NULL; >> + } >> + } >> + >> + for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) { >> + if (ifm->ifq_so == so) { >> + ifm->ifq_so = NULL; >> + } >> + } >> +} >> + >> /* >> * remque and free a socket, clobber cache >> */ >> @@ -79,6 +106,8 @@ sofree(struct socket *so) >> if(so->so_next && so->so_prev) >> remque(so); /* crashes if so is not in a queue */ >> >> + soremovefromqueues(so); >> + >> free(so); >> } >> >> > Sorry for the late feedback, > Jan >
On 2013-02-22 10:57, Vitaly Chipounov wrote: > Hi, > > On 21.02.2013 15:33, Jan Kiszka wrote: >> On 2013-02-15 12:00, Vitaly Chipounov wrote: >>> A socket may still have references to it in various queues >>> at the time it is freed, causing memory corruptions. >> Did you see it in practice? Or is this patch based on code review? What >> will happen if those queued mbufs find their ifq_so NULL? > > I have a packet trace that triggers this problem when it is injected > into the guest's NIC. > I am still not quite sure why this happens, but suspect that it could be > caused by malformed/partial TCP/IP streams. OK. Is it shareable? We need to understand why these bufs still hang around - and exclude they leak or cause more problems elsewhere. Jan
diff --git a/slirp/socket.c b/slirp/socket.c index 77b0c98..8a7adc8 100644 --- a/slirp/socket.c +++ b/slirp/socket.c @@ -55,6 +55,33 @@ socreate(Slirp *slirp) return(so); } +/** + * It may happen that a socket is still referenced in various + * mbufs at the time it is freed. Clear all references to the + * socket here. + */ +static void soremovefromqueues(struct socket *so) +{ + if (!so->so_queued) { + return; + } + + Slirp *slirp = so->slirp; + struct mbuf *ifm; + + for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) { + if (ifm->ifq_so == so) { + ifm->ifq_so = NULL; + } + } + + for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) { + if (ifm->ifq_so == so) { + ifm->ifq_so = NULL; + } + } +} + /* * remque and free a socket, clobber cache */ @@ -79,6 +106,8 @@ sofree(struct socket *so) if(so->so_next && so->so_prev) remque(so); /* crashes if so is not in a queue */ + soremovefromqueues(so); + free(so); }
A socket may still have references to it in various queues at the time it is freed, causing memory corruptions. Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch> --- slirp/socket.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)