xenstored file descriptor leak

xenstored file descriptor leak

[Manuel Bouyer]

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).

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.
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);
 		}
 	}

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);
 }
 
 static void handle_output(struct connection *conn)


what do you think ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2444 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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 ?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3415 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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 

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 973 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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

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.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

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

--- xenstored_core.c.orig	2021-02-02 18:06:33.389316841 +0100
+++ xenstored_core.c	2021-02-03 12:46:17.204376338 +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);
 		}
 	}
@@ -1440,6 +1443,9 @@
 
 	talloc_free(conn->in);
 	conn->in = NULL;
+	/* if this is a socket connection, drop it now */
+	if (conn->fd >= 0)
+		talloc_free(conn);
 }
 
 static const char *sockmsg_string(enum xsd_sockmsg_type type)

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 872 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1197 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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>
?

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1628 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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

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).

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

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

--- xenstored_core.c.orig	2021-02-03 13:25:38.568308454 +0100
+++ xenstored_core.c	2021-02-03 13:25:44.282429872 +0100
@@ -1440,6 +1440,9 @@
 
 	talloc_free(conn->in);
 	conn->in = NULL;
+	/* if this is a socket connection, drop it now */
+	if (conn->fd >= 0)
+		talloc_free(conn);
 }
 
 static const char *sockmsg_string(enum xsd_sockmsg_type type)

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1244 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 472 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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 ...

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Re: xenstored file descriptor leak

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 878 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: xenstored file descriptor leak

[Manuel Bouyer]

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 ...

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--