Xen devel - Sep 2012 - [PATCH] libxenstore: filter watch events in libxenstore when we unwatch

XenStore puts in queued watch events via a thread and notifies the user.
Sometimes xs_unwatch is called before all related message is read. The use
case is non-threaded libevent, we have two event A and B:
    - Event A will destroy something and call xs_unwatch;
    - Event B is used to notify that a node has changed in XenStore.
As the event is called one by one, event A can be handled before event B.
So on next xs_watch_read the user could retrieve an unwatch token and
a segfault occured if the token store the pointer of the structure
(ie: "backend:0xcafe").

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 tools/xenstore/xs.c |   41 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index b951015..31aad14 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -855,14 +855,53 @@ char **xs_read_watch(struct xs_handle *h, unsigned int
*num)
 bool xs_unwatch(struct xs_handle *h, const char *path, const char *token)
 {
 	struct iovec iov[2];
+	struct xs_stored_msg *msg, *tmsg;
+	bool res;
+	char *strings;
+	unsigned int num_strings, i;
+	char c;
 
 	iov[0].iov_base = (char *)path;
 	iov[0].iov_len = strlen(path) + 1;
 	iov[1].iov_base = (char *)token;
 	iov[1].iov_len = strlen(token) + 1;
 
-	return xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
+	res = xs_bool(xs_talkv(h, XBT_NULL, XS_UNWATCH, iov,
 				ARRAY_SIZE(iov), NULL));
+
+	/* Filter the watch list to remove potential message */
+	mutex_lock(&h->watch_mutex);
+
+	if (list_empty(&h->watch_list)) {
+		mutex_unlock(&h->watch_mutex);
+		return res;
+	}
+
+	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
+		assert(msg->hdr.type == XS_WATCH_EVENT);
+
+		strings = msg->body;
+		num_strings = xs_count_strings(strings, msg->hdr.len);
+
+		for (i = 0; i < num_strings; i++) {
+			if (i == XS_WATCH_TOKEN && !strcmp (token, strings)) {
+				list_del(&msg->list);
+				free(msg);
+				break;
+			}
+			strings = strings + strlen (strings) + 1;
+		}
+	}
+
+	/* Clear the pipe token if there are no more pending watches. */
+	if (list_empty(&h->watch_list) && (h->watch_pipe[0] != -1))
{
+		while (read(h->watch_pipe[0], &c, 1) != 1)
+			continue;
+	}
+
+	mutex_unlock(&h->watch_mutex);
+
+	return res;
 }
 
 /* Start a transaction: changes by others will not be seen during this
-- 
Julien Grall

Julien Grall writes ("[PATCH] libxenstore: filter watch events in
libxenstore when we unwatch"):
> XenStore puts in queued watch events via a thread and notifies the user.
> Sometimes xs_unwatch is called before all related message is read. The use
> case is non-threaded libevent, we have two event A and B:
>     - Event A will destroy something and call xs_unwatch;
>     - Event B is used to notify that a node has changed in XenStore.
> As the event is called one by one, event A can be handled before event B.
> So on next xs_watch_read the user could retrieve an unwatch token and
> a segfault occured if the token store the pointer of the structure
> (ie: "backend:0xcafe").
Missing from the explanation here is why your patch is sufficient to
avoid the race.  The answer is as follows (and should probably be in
the commit message):

While on entry to xs_unwatch, there may be an event on its way from
xenstored (eg in the ring or in the local kernel), all such events
will definitely come before the reply to the unwatch command.  So at
the point where the unwatch reply has been processed (after xs_talkv),
any such now-deleted watch events will definitely have made it to
libxenstore''s queue where we can remove them.

As for other threads in the same process: if two threads call
xs_read_watch and xs_unwatch, it is acceptable for the xs_read_watch
to return the event being deleted.  What is not allowed is for an
xs_read_watch entered after xs_unwatch returns to return the deleted
event, and this code does indeed ensure that.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
(for the explanation)

Now for some comments on the patch:
> +	/* Filter the watch list to remove potential message */
> +	mutex_lock(&h->watch_mutex);
> +
> +	if (list_empty(&h->watch_list)) {
> +		mutex_unlock(&h->watch_mutex);
> +		return res;
> +	}
I think this check is unnecessary.  If the list is empty then walking
it is trivially a no-op.
> +	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
> +		assert(msg->hdr.type == XS_WATCH_EVENT);
> +
> +		strings = msg->body;
> +		num_strings = xs_count_strings(strings, msg->hdr.len);
The num_strings thing is obsolete.  There will always be two strings.
Also xs_count_strings walks the array.
> +		for (i = 0; i < num_strings; i++) {
> +			if (i == XS_WATCH_TOKEN && !strcmp (token, strings)) {
This is rather odd.  It amounts to:

   for (i= blah blah) { if (i==FIXED_VALUE) { ..i.. } }
> +				list_del(&msg->list);
> +				free(msg);
> +				break;
> +			}
> +			strings = strings + strlen (strings) + 1;
You need to check the path as well as the token, since it is legal to
set up multiple watches on different paths with the same token.

I think you can then do away with the calculation of "strings", at
least mostly.
> +	/* Clear the pipe token if there are no more pending watches. */
> +	if (list_empty(&h->watch_list) && (h->watch_pipe[0] !=
-1)) {
> +		while (read(h->watch_pipe[0], &c, 1) != 1)
> +			continue;
> +	}
I''m not convinced this is necessary.  Don''t callers already
need to
cope with potential spurious signallings of the watch pipe ?

In any case it''s clone-and-hack: the same 3/4 lines occur elsewhere.
If we keep this in the patch it should be made into a function.

Thanks,
Ian.

On 09/21/2012 04:45 PM, Ian Jackson wrote:
> Julien Grall writes ("[PATCH] libxenstore: filter watch events in
libxenstore when we unwatch"):
>> XenStore puts in queued watch events via a thread and notifies the
user.
>> Sometimes xs_unwatch is called before all related message is read. The
use
>> case is non-threaded libevent, we have two event A and B:
>>     - Event A will destroy something and call xs_unwatch;
>>     - Event B is used to notify that a node has changed in XenStore.
>> As the event is called one by one, event A can be handled before event
B.
>> So on next xs_watch_read the user could retrieve an unwatch token and
>> a segfault occured if the token store the pointer of the structure
>> (ie: "backend:0xcafe").
> 
> Missing from the explanation here is why your patch is sufficient to
> avoid the race.  The answer is as follows (and should probably be in
> the commit message):
> 
> While on entry to xs_unwatch, there may be an event on its way from
> xenstored (eg in the ring or in the local kernel), all such events
> will definitely come before the reply to the unwatch command.  So at
> the point where the unwatch reply has been processed (after xs_talkv),
> any such now-deleted watch events will definitely have made it to
> libxenstore''s queue where we can remove them.
> 
> As for other threads in the same process: if two threads call
> xs_read_watch and xs_unwatch, it is acceptable for the xs_read_watch
> to return the event being deleted.  What is not allowed is for an
> xs_read_watch entered after xs_unwatch returns to return the deleted
> event, and this code does indeed ensure that.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> (for the explanation)
> 
> Now for some comments on the patch:
> 
>> +	/* Filter the watch list to remove potential message */
>> +	mutex_lock(&h->watch_mutex);
>> +
>> +	if (list_empty(&h->watch_list)) {
>> +		mutex_unlock(&h->watch_mutex);
>> +		return res;
>> +	}
> 
> I think this check is unnecessary.  If the list is empty then walking
> it is trivially a no-op.
It''s usefull, if we need to clean the pipe (see explanation below).
Otherwise, the read will block and we want to avoid that on
mono-threaded (I don''t consider xenstore thread).
>> +	list_for_each_entry_safe(msg, tmsg, &h->watch_list, list) {
>> +		assert(msg->hdr.type == XS_WATCH_EVENT);
>> +
>> +		strings = msg->body;
>> +		num_strings = xs_count_strings(strings, msg->hdr.len);
> 
> The num_strings thing is obsolete.  There will always be two strings.
> Also xs_count_strings walks the array.
We can''t assume that XS_WATCH_EVENT will always equal to 2. So we need
to browse until we find the right string. I use xs_count_strings because
it allows to not take care of the length message in the loop.
>> +		for (i = 0; i < num_strings; i++) {
>> +			if (i == XS_WATCH_TOKEN && !strcmp (token, strings)) {
> 
> This is rather odd.  It amounts to:
> 
>    for (i= blah blah) { if (i==FIXED_VALUE) { ..i.. } }
> 
>> +				list_del(&msg->list);
>> +				free(msg);
>> +				break;
>> +			}
>> +			strings = strings + strlen (strings) + 1;
> 
> You need to check the path as well as the token, since it is legal to
> set up multiple watches on different paths with the same token.
> 
> I think you can then do away with the calculation of "strings",
at
> least mostly.
> 
>> +	/* Clear the pipe token if there are no more pending watches. */
>> +	if (list_empty(&h->watch_list) && (h->watch_pipe[0]
!= -1)) {
>> +		while (read(h->watch_pipe[0], &c, 1) != 1)
>> +			continue;
>> +	}
> 
> I''m not convinced this is necessary.  Don''t callers
already need to
> cope with potential spurious signallings of the watch pipe ?

I base my code on xs_read_watch. As I understand xs_read_watch, it will
wait on a condition until the list is not empty.
So if the list is empty and not the pipe, an event can occur and block
the application (with xs_read_watch).

Sincerely yours,

Julien Grall

Julien Grall writes ("Re: [PATCH] libxenstore: filter watch events in
libxenstore when we unwatch"):
> On 09/21/2012 04:45 PM, Ian Jackson wrote:
> > The num_strings thing is obsolete.  There will always be two strings.
> > Also xs_count_strings walks the array.
> 
> We can''t assume that XS_WATCH_EVENT will always equal to 2. So we
need
> to browse until we find the right string. I use xs_count_strings because
> it allows to not take care of the length message in the loop.
You mean XS_WATCH_TOKEN.  And yes, we can assume that XS_WATCH_TOKEN
will always equal 1.  What is obsolete is the idea that it might be
anything different.
> >> +	/* Clear the pipe token if there are no more pending watches. */
> >> +	if (list_empty(&h->watch_list) &&
(h->watch_pipe[0] != -1)) {
> >> +		while (read(h->watch_pipe[0], &c, 1) != 1)
> >> +			continue;
> >> +	}
> > 
> > I''m not convinced this is necessary.  Don''t callers
already need to
> > cope with potential spurious signallings of the watch pipe ?
> 
> I base my code on xs_read_watch. As I understand xs_read_watch, it will
> wait on a condition until the list is not empty.
> So if the list is empty and not the pipe, an event can occur and block
> the application (with xs_read_watch).
xs_read_watch can only be correctly be used if you are determined to
wait, right there, until a particular watch turns up.

If you are using xs_fileno, you must do as the comment says:
 * Callers should, after xs_fileno has become readable, repeatedly
 * call xs_check_watch until it returns NULL and sets errno to EAGAIN.
 * (If the fd became readable, xs_check_watch is allowed to make it no
 * longer show up as readable even if future calls to xs_check_watch
 * will return more watch events.)

I agree that the comments in xenstore.h about this are less than
clear.

Ian.

On Fri, Sep 21, 2012 at 5:28 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Julien Grall writes ("Re: [PATCH] libxenstore: filter watch events in
libxenstore when we unwatch"):
>> On 09/21/2012 04:45 PM, Ian Jackson wrote:
>> > The num_strings thing is obsolete.  There will always be two
strings.
>> > Also xs_count_strings walks the array.
>>
>> >> +  /* Clear the pipe token if there are no more pending
watches. */
>> >> +  if (list_empty(&h->watch_list) &&
(h->watch_pipe[0] != -1)) {
>> >> +          while (read(h->watch_pipe[0], &c, 1) != 1)
>> >> +                  continue;
>> >> +  }
>> >
>> > I''m not convinced this is necessary.  Don''t
callers already need to
>> > cope with potential spurious signallings of the watch pipe ?
>>
>> I base my code on xs_read_watch. As I understand xs_read_watch, it will
>> wait on a condition until the list is not empty.
>> So if the list is empty and not the pipe, an event can occur and block
>> the application (with xs_read_watch).
>
> xs_read_watch can only be correctly be used if you are determined to
> wait, right there, until a particular watch turns up.
>
> If you are using xs_fileno, you must do as the comment says:
>  * Callers should, after xs_fileno has become readable, repeatedly
>  * call xs_check_watch until it returns NULL and sets errno to EAGAIN.
>  * (If the fd became readable, xs_check_watch is allowed to make it no
>  * longer show up as readable even if future calls to xs_check_watch
>  * will return more watch events.)
The function xs_check_watch was recently introduced in Xen (see your
commit on december 2011).
Most of application, for instance QEMU, still use xs_read_watch.
So if xs_unwatch doesn''t empty the pipe, most of this applications
potentially stall indefinitly.

IMHO, this piece of code is really important.

Sincerely yours,

-- 
Grall Julien

Julien Grall writes ("Re: [Xen-devel] [PATCH] libxenstore: filter watch
events in libxenstore when we unwatch"):
> The function xs_check_watch was recently introduced in Xen (see your
> commit on december 2011).
Oh, I''d forgotten that.
> Most of application, for instance QEMU, still use xs_read_watch.
> So if xs_unwatch doesn''t empty the pipe, most of this applications
> potentially stall indefinitly.
> 
> IMHO, this piece of code is really important.
In that case, I agree.

Thanks,
Ian.