| Message ID | 20210202183735.GA25046@mail.soc.lip6.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xenstored file descriptor leak | expand |
On 02.02.21 19:37, Manuel Bouyer wrote: > Hello, > on NetBSD I'm tracking down an issue where xenstored never closes its > file descriptor to /var/run/xenstored/socket and instead loops at 100% > CPU on these descriptors. > > xenstored loops because poll(2) returns a POLLIN event for these > descriptors but they are marked is_ignored = true. > > I'm seeing this with xen 4.15, 4.13 and has also been reported with 4.11 > with latest security patches. > It seems to have started with patches for XSA-115 (A user reported this > for 4.11) > > I've tracked it down to a difference in poll(2) implementation; it seems > that linux will return something that is not (POLLIN|POLLOUT) when a > socket if closed; while NetBSD returns POLLIN (this matches the NetBSD's > man page). Yeah, Linux seems to return POLLHUP additionally. > > First I think there may be a security issue here, as, even on linux it should > be possible for a client to cause a socket to go to the is_ignored state, > while still sending data and cause xenstored to go to a 100% CPU loop. No security issue, as sockets are restricted to dom0 and user root. In case you are suspecting a security issue, please don't send such mails to xen-devel in future, but to security@lists.xenproject.org. > I think this is needed anyway: > > --- xenstored_core.c.orig 2021-02-02 18:06:33.389316841 +0100 > +++ xenstored_core.c 2021-02-02 19:27:43.761877371 +0100 > @@ -397,9 +397,12 @@ > !list_empty(&conn->out_list))) > *ptimeout = 0; > } else { > - short events = POLLIN|POLLPRI; > - if (!list_empty(&conn->out_list)) > - events |= POLLOUT; > + short events = 0; > + if (!conn->is_ignored) { > + events |= POLLIN|POLLPRI; > + if (!list_empty(&conn->out_list)) > + events |= POLLOUT; > + } > conn->pollfd_idx = set_fd(conn->fd, events); > } > } Yes, I think this is a good idea. > > Now I wonder if, on NetBSD at last, a read error or short read shouldn't > cause the socket to be closed, as with: > > @@ -1561,6 +1565,8 @@ > > bad_client: > ignore_connection(conn); > + /* we don't want to keep this connection alive */ > + talloc_free(conn); > } This is wrong for non-socket connections, as we want to keep the domain in question to be known to xenstored. For socket connections this should be okay, though. Juergen
On Wed, Feb 03, 2021 at 07:18:51AM +0100, Jürgen Groß wrote: > On 02.02.21 19:37, Manuel Bouyer wrote: > > Hello, > > on NetBSD I'm tracking down an issue where xenstored never closes its > > file descriptor to /var/run/xenstored/socket and instead loops at 100% > > CPU on these descriptors. > > > > xenstored loops because poll(2) returns a POLLIN event for these > > descriptors but they are marked is_ignored = true. > > > > I'm seeing this with xen 4.15, 4.13 and has also been reported with 4.11 > > with latest security patches. > > It seems to have started with patches for XSA-115 (A user reported this > > for 4.11) > > > > I've tracked it down to a difference in poll(2) implementation; it seems > > that linux will return something that is not (POLLIN|POLLOUT) when a > > socket if closed; while NetBSD returns POLLIN (this matches the NetBSD's > > man page). > > Yeah, Linux seems to return POLLHUP additionally. Overall, it seems that the poll(2) behavior with non-regular files is highly OS-dependant when it comes to border cases (like a remote close of a socket) > > > > > First I think there may be a security issue here, as, even on linux it should > > be possible for a client to cause a socket to go to the is_ignored state, > > while still sending data and cause xenstored to go to a 100% CPU loop. > > No security issue, as sockets are restricted to dom0 and user root. > > In case you are suspecting a security issue, please don't send such > mails to xen-devel in future, but to security@lists.xenproject.org. Yes, sorry. Will do next time. > > > I think this is needed anyway: > > > > --- xenstored_core.c.orig 2021-02-02 18:06:33.389316841 +0100 > > +++ xenstored_core.c 2021-02-02 19:27:43.761877371 +0100 > > @@ -397,9 +397,12 @@ > > !list_empty(&conn->out_list))) > > *ptimeout = 0; > > } else { > > - short events = POLLIN|POLLPRI; > > - if (!list_empty(&conn->out_list)) > > - events |= POLLOUT; > > + short events = 0; > > + if (!conn->is_ignored) { > > + events |= POLLIN|POLLPRI; > > + if (!list_empty(&conn->out_list)) > > + events |= POLLOUT; > > + } > > conn->pollfd_idx = set_fd(conn->fd, events); > > } > > } > > Yes, I think this is a good idea. Well, after some sleep I don't think it is. We should always keep at last POLLIN or we will never notice a socket close otherwise. > > > > > Now I wonder if, on NetBSD at last, a read error or short read shouldn't > > cause the socket to be closed, as with: > > > > @@ -1561,6 +1565,8 @@ > > bad_client: > > ignore_connection(conn); > > + /* we don't want to keep this connection alive */ > > + talloc_free(conn); > > } > > This is wrong for non-socket connections, as we want to keep the domain > in question to be known to xenstored. > > For socket connections this should be okay, though. What are "non-socket connections" BTW ? I don't think I've seen one in my test. Is there a way to know if a connection is socket or non-socket ?
On 03.02.21 08:57, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 07:18:51AM +0100, Jürgen Groß wrote: >> On 02.02.21 19:37, Manuel Bouyer wrote: >>> Hello, >>> on NetBSD I'm tracking down an issue where xenstored never closes its >>> file descriptor to /var/run/xenstored/socket and instead loops at 100% >>> CPU on these descriptors. >>> >>> xenstored loops because poll(2) returns a POLLIN event for these >>> descriptors but they are marked is_ignored = true. >>> >>> I'm seeing this with xen 4.15, 4.13 and has also been reported with 4.11 >>> with latest security patches. >>> It seems to have started with patches for XSA-115 (A user reported this >>> for 4.11) >>> >>> I've tracked it down to a difference in poll(2) implementation; it seems >>> that linux will return something that is not (POLLIN|POLLOUT) when a >>> socket if closed; while NetBSD returns POLLIN (this matches the NetBSD's >>> man page). >> >> Yeah, Linux seems to return POLLHUP additionally. > > Overall, it seems that the poll(2) behavior with non-regular files > is highly OS-dependant when it comes to border cases (like a remote > close of a socket) > >> >>> >>> First I think there may be a security issue here, as, even on linux it should >>> be possible for a client to cause a socket to go to the is_ignored state, >>> while still sending data and cause xenstored to go to a 100% CPU loop. >> >> No security issue, as sockets are restricted to dom0 and user root. >> >> In case you are suspecting a security issue, please don't send such >> mails to xen-devel in future, but to security@lists.xenproject.org. > > Yes, sorry. Will do next time. > >> >>> I think this is needed anyway: >>> >>> --- xenstored_core.c.orig 2021-02-02 18:06:33.389316841 +0100 >>> +++ xenstored_core.c 2021-02-02 19:27:43.761877371 +0100 >>> @@ -397,9 +397,12 @@ >>> !list_empty(&conn->out_list))) >>> *ptimeout = 0; >>> } else { >>> - short events = POLLIN|POLLPRI; >>> - if (!list_empty(&conn->out_list)) >>> - events |= POLLOUT; >>> + short events = 0; >>> + if (!conn->is_ignored) { >>> + events |= POLLIN|POLLPRI; >>> + if (!list_empty(&conn->out_list)) >>> + events |= POLLOUT; >>> + } >>> conn->pollfd_idx = set_fd(conn->fd, events); >>> } >>> } >> >> Yes, I think this is a good idea. > > Well, after some sleep I don't think it is. We should always keep at last > POLLIN or we will never notice a socket close otherwise. Adding the fd of an ignored socket connection to the list is the real problem here. Why should that be done? > >> >>> >>> Now I wonder if, on NetBSD at last, a read error or short read shouldn't >>> cause the socket to be closed, as with: >>> >>> @@ -1561,6 +1565,8 @@ >>> bad_client: >>> ignore_connection(conn); >>> + /* we don't want to keep this connection alive */ >>> + talloc_free(conn); >>> } >> >> This is wrong for non-socket connections, as we want to keep the domain >> in question to be known to xenstored. >> >> For socket connections this should be okay, though. > > What are "non-socket connections" BTW ? I don't think I've seen one > in my test. Every connection to another domain. > Is there a way to know if a connection is socket or non-socket ? Active socket connections have conn->fd >= 0. Juergen
On Wed, Feb 03, 2021 at 09:05:27AM +0100, Jürgen Groß wrote: > > > [...] > > > Yes, I think this is a good idea. > > > > Well, after some sleep I don't think it is. We should always keep at last > > POLLIN or we will never notice a socket close otherwise. > > Adding the fd of an ignored socket connection to the list is the real > problem here. Why should that be done? If we don't do it, we never notice when the socket is closed and the file descriptor will stay forever. When I tried it, I had about 50 zombie file descriptors open in xenstored, after starting only 2 domains. > > > > > > > > Now I wonder if, on NetBSD at last, a read error or short read shouldn't > > > > cause the socket to be closed, as with: > > > > > > > > @@ -1561,6 +1565,8 @@ > > > > bad_client: > > > > ignore_connection(conn); > > > > + /* we don't want to keep this connection alive */ > > > > + talloc_free(conn); > > > > } > > > > > > This is wrong for non-socket connections, as we want to keep the domain > > > in question to be known to xenstored. > > > > > > For socket connections this should be okay, though. > > > > What are "non-socket connections" BTW ? I don't think I've seen one > > in my test. > > Every connection to another domain. > > > Is there a way to know if a connection is socket or non-socket ? > > Active socket connections have conn->fd >= 0. OK, I'll rework my patch. Thanks
On 03.02.21 09:16, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 09:05:27AM +0100, Jürgen Groß wrote: >>>> [...] >>>> Yes, I think this is a good idea. >>> >>> Well, after some sleep I don't think it is. We should always keep at last >>> POLLIN or we will never notice a socket close otherwise. >> >> Adding the fd of an ignored socket connection to the list is the real >> problem here. Why should that be done? > > If we don't do it, we never notice when the socket is closed and the file > descriptor will stay forever. When I tried it, I had about 50 zombie > file descriptors open in xenstored, after starting only 2 domains. This shouldn't happen in case we are closing the socket actively. In the end we should just do a talloc_free(conn) in ignore_connection() if it is a socket based one. This should revert the critical modification of the XSA-115 fixes for sockets while keeping the desired effect for domain connections. Juergen
On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote: > [...] > This shouldn't happen in case we are closing the socket actively. > > In the end we should just do a talloc_free(conn) in > ignore_connection() if it is a socket based one. This should revert > the critical modification of the XSA-115 fixes for sockets while > keeping the desired effect for domain connections. Hello here's an updated patch which works for me. Does anyone see a problem with it ? If not I will submit it for commit.
On 03.02.21 12:48, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote: >> [...] >> This shouldn't happen in case we are closing the socket actively. >> >> In the end we should just do a talloc_free(conn) in >> ignore_connection() if it is a socket based one. This should revert >> the critical modification of the XSA-115 fixes for sockets while >> keeping the desired effect for domain connections. > > Hello > here's an updated patch which works for me. Does anyone see a problem > with it ? If not I will submit it for commit. > Do you really need the first hunk? I would have thought just freeing conn in ignore_connection() is enough. In case you are seeing problems without the first hunk, please say so in a comment added to this hunk in order to avoid it being removed sometimes in the future. Juergen
On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote: > On 03.02.21 12:48, Manuel Bouyer wrote: > > On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote: > > > [...] > > > This shouldn't happen in case we are closing the socket actively. > > > > > > In the end we should just do a talloc_free(conn) in > > > ignore_connection() if it is a socket based one. This should revert > > > the critical modification of the XSA-115 fixes for sockets while > > > keeping the desired effect for domain connections. > > > > Hello > > here's an updated patch which works for me. Does anyone see a problem > > with it ? If not I will submit it for commit. > > > > Do you really need the first hunk? I would have thought just freeing > conn in ignore_connection() is enough. > > In case you are seeing problems without the first hunk, please say so > in a comment added to this hunk in order to avoid it being removed > sometimes in the future. No I don't need it. From your previous comments I though it was a good idea to keep it, but I can remove it if you think it's better.
On 03.02.21 13:03, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote: >> On 03.02.21 12:48, Manuel Bouyer wrote: >>> On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote: >>>> [...] >>>> This shouldn't happen in case we are closing the socket actively. >>>> >>>> In the end we should just do a talloc_free(conn) in >>>> ignore_connection() if it is a socket based one. This should revert >>>> the critical modification of the XSA-115 fixes for sockets while >>>> keeping the desired effect for domain connections. >>> >>> Hello >>> here's an updated patch which works for me. Does anyone see a problem >>> with it ? If not I will submit it for commit. >>> >> >> Do you really need the first hunk? I would have thought just freeing >> conn in ignore_connection() is enough. >> >> In case you are seeing problems without the first hunk, please say so >> in a comment added to this hunk in order to avoid it being removed >> sometimes in the future. > > No I don't need it. From your previous comments I though it was a good idea > to keep it, but I can remove it if you think it's better. Yes, please remove it. Juergen
On Wed, Feb 03, 2021 at 01:13:53PM +0100, Jürgen Groß wrote: > On 03.02.21 13:03, Manuel Bouyer wrote: > > On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote: > > > On 03.02.21 12:48, Manuel Bouyer wrote: > > > > On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote: > > > > > [...] > > > > > This shouldn't happen in case we are closing the socket actively. > > > > > > > > > > In the end we should just do a talloc_free(conn) in > > > > > ignore_connection() if it is a socket based one. This should revert > > > > > the critical modification of the XSA-115 fixes for sockets while > > > > > keeping the desired effect for domain connections. > > > > > > > > Hello > > > > here's an updated patch which works for me. Does anyone see a problem > > > > with it ? If not I will submit it for commit. > > > > > > > > > > Do you really need the first hunk? I would have thought just freeing > > > conn in ignore_connection() is enough. > > > > > > In case you are seeing problems without the first hunk, please say so > > > in a comment added to this hunk in order to avoid it being removed > > > sometimes in the future. > > > > No I don't need it. From your previous comments I though it was a good idea > > to keep it, but I can remove it if you think it's better. > > Yes, please remove it. Will do In the patch I'm going to submit, may I add Reviewed-by: Jürgen Groß <jgross@suse.com> ?
On 03.02.21 13:17, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 01:13:53PM +0100, Jürgen Groß wrote: >> On 03.02.21 13:03, Manuel Bouyer wrote: >>> On Wed, Feb 03, 2021 at 12:54:23PM +0100, Jürgen Groß wrote: >>>> On 03.02.21 12:48, Manuel Bouyer wrote: >>>>> On Wed, Feb 03, 2021 at 09:21:32AM +0100, Jürgen Groß wrote: >>>>>> [...] >>>>>> This shouldn't happen in case we are closing the socket actively. >>>>>> >>>>>> In the end we should just do a talloc_free(conn) in >>>>>> ignore_connection() if it is a socket based one. This should revert >>>>>> the critical modification of the XSA-115 fixes for sockets while >>>>>> keeping the desired effect for domain connections. >>>>> >>>>> Hello >>>>> here's an updated patch which works for me. Does anyone see a problem >>>>> with it ? If not I will submit it for commit. >>>>> >>>> >>>> Do you really need the first hunk? I would have thought just freeing >>>> conn in ignore_connection() is enough. >>>> >>>> In case you are seeing problems without the first hunk, please say so >>>> in a comment added to this hunk in order to avoid it being removed >>>> sometimes in the future. >>> >>> No I don't need it. From your previous comments I though it was a good idea >>> to keep it, but I can remove it if you think it's better. >> >> Yes, please remove it. > > Will do > > In the patch I'm going to submit, may I add > > Reviewed-by: Jürgen Groß <jgross@suse.com> > ? > Let me see the patch (including commit message) first, please. So just send out as a regular patch, and I'll respond accordingly. :-) Juergen
On Wed, Feb 03, 2021 at 01:21:59PM +0100, Jürgen Groß wrote: > > Will do > > > > In the patch I'm going to submit, may I add > > > > Reviewed-by: Jürgen Groß <jgross@suse.com> > > ? > > > > Let me see the patch (including commit message) first, please. > > So just send out as a regular patch, and I'll respond accordingly. :-) Attached is the new patch. As commit message I suggest: xenstored: close socket connections on error On error, don't keep socket connection in ignored state but close them. When the remote end of a socket is closed, xenstored will flag it as an error and switch the connection to ignored. But on some OSes (e.g. NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored state will stay open forever in xenstored (and it will loop with CPU 100% busy).
On 03.02.21 13:33, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 01:21:59PM +0100, Jürgen Groß wrote: >>> Will do >>> >>> In the patch I'm going to submit, may I add >>> >>> Reviewed-by: Jürgen Groß <jgross@suse.com> >>> ? >>> >> >> Let me see the patch (including commit message) first, please. >> >> So just send out as a regular patch, and I'll respond accordingly. :-) > > Attached is the new patch. > > As commit message I suggest: > > xenstored: close socket connections on error > > On error, don't keep socket connection in ignored state but close them. > When the remote end of a socket is closed, xenstored will flag it as an > error and switch the connection to ignored. But on some OSes (e.g. > NetBSD), poll(2) will return only POLLIN in this case, so sockets in ignored > state will stay open forever in xenstored (and it will loop with CPU 100% > busy). > Uh, this is no regular patch. You've sent correct patches before, haven't you? The patch should have a Signed-off-by: and in this case a Fixes: tag as well (hint: the patch breaking your use case was commit d2fa370d3ef9cbe22d7256c608671cdcdf6e0083). See https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches Juergen
On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote: > Uh, this is no regular patch. I though by regular patch, you meants a plain diff -u > You've sent correct patches before, Yes, and it's very time-consuming. This is why I want to have it right the first time and not go through sevreral iterations with this protocol.
On 03.02.21 13:47, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote: >> Uh, this is no regular patch. > > I though by regular patch, you meants a plain diff -u > >> You've sent correct patches before, > > Yes, and it's very time-consuming. This is why I want to have it right the > first time and not go through sevreral iterations with this protocol. > Its not that hard if you have a proper git tree... Juergen
On Wed, Feb 03, 2021 at 01:58:19PM +0100, Jürgen Groß wrote: > On 03.02.21 13:47, Manuel Bouyer wrote: > > On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote: > > > Uh, this is no regular patch. > > > > I though by regular patch, you meants a plain diff -u > > > > > You've sent correct patches before, > > > > Yes, and it's very time-consuming. This is why I want to have it right the > > first time and not go through sevreral iterations with this protocol. > > > > Its not that hard if you have a proper git tree... git is the problem actually. I'm not used to it ...
On 03.02.21 14:03, Manuel Bouyer wrote: > On Wed, Feb 03, 2021 at 01:58:19PM +0100, Jürgen Groß wrote: >> On 03.02.21 13:47, Manuel Bouyer wrote: >>> On Wed, Feb 03, 2021 at 01:42:12PM +0100, Jürgen Groß wrote: >>>> Uh, this is no regular patch. >>> >>> I though by regular patch, you meants a plain diff -u >>> >>>> You've sent correct patches before, >>> >>> Yes, and it's very time-consuming. This is why I want to have it right the >>> first time and not go through sevreral iterations with this protocol. >>> >> >> Its not that hard if you have a proper git tree... > > git is the problem actually. I'm not used to it ... > Not using git on a daily basis I really suggest to use stgit as an add.on: https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_StGit It makes handling multiple iterations of a patch rather easy. Juergen
On Wed, Feb 03, 2021 at 02:11:43PM +0100, Jürgen Groß wrote: > Not using git on a daily basis I really suggest to use stgit as an > add.on: > > https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_StGit > > It makes handling multiple iterations of a patch rather easy. thanks. When I started, I looked at the wiki for instructions about patches, but didn't find any ...
--- xenstored_core.c.orig 2021-02-02 18:06:33.389316841 +0100 +++ xenstored_core.c 2021-02-02 19:27:43.761877371 +0100 @@ -397,9 +397,12 @@ !list_empty(&conn->out_list))) *ptimeout = 0; } else { - short events = POLLIN|POLLPRI; - if (!list_empty(&conn->out_list)) - events |= POLLOUT; + short events = 0; + if (!conn->is_ignored) { + events |= POLLIN|POLLPRI; + if (!list_empty(&conn->out_list)) + events |= POLLOUT; + } conn->pollfd_idx = set_fd(conn->fd, events); } }