Re: [Xen-devel] [PATCH v8.1 17/27] xsplice: Add support for bug frames.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Apr 21, 2016 at 12:49:09AM -0600, Jan Beulich wrote:
> >>> On 21.04.16 at 02:29, <konrad.wilk@xxxxxxxxxx> wrote:
> > On Tue, Apr 19, 2016 at 02:17:35PM -0600, Jan Beulich wrote:
> >> >>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:02 AM >>>
> >> >+bool_t is_patch(const void *ptr)
> >> >+{
> >> >+    struct payload *data;
> >> 
> >> You guess it: const.
> >> 
> >> >+    /*
> >> >+     * No locking since this list is only ever changed during apply or 
> >> >revert
> >> >+     * context.
> >> >+     */
> >> 
> >> What if you crash while applying or reverting a patch? Is the list update 
> >> at
> >> least done such that the (then nested) traversal remains safe?
> > 
> > Yes! We only add the struct payload to this applied_list _after_ the
> > patching has been done.` Hence if we crashed the list would not contain the
> > struct payload that was patching  - but would be safe to traverse.
> 
> Well, that only partly answers the question. Patch application to me
> means the entire process, i.e. up to and including adding the new
> patch to the list (read: the crash could also happen there). Hence
> I'd still like it to be made sure that even if the addition has got done
> only partially, the list remains traversable. Or IOW, I'm not sure the
> standard list macros make any guarantees like that.

It does. I've copied-n-pasted the list_add_tail hacking it have an
BUG_ON(1):

+static inline void __list_add_crash(struct list_head *new,
+                              struct list_head *prev, /* applied list */
+                              struct list_head *next /* applied_list */)
+{
+    next->prev = new; /* applied_list->prev = new */
+    new->next = next; /* new->next = applied_list */
+    new->prev = prev; /* new->prev = applied_list */
+    BUG_ON(1);
+    prev->next = new; /* applied_list->next=new */
+}
+static inline void list_add_tail_crash(struct list_head *new, struct
list_head *head)
+{
+    __list_add_crash(new, head->prev /* applied_list */, head /* applied_list 
*/);
+}
 static int apply_payload(struct payload *data)
 {
     unsigned int i;
@@ -967,7 +981,7 @@ static int apply_payload(struct payload *data)
 
     arch_xsplice_patching_leave();
 
-    list_add_tail(&data->applied_list, &applied_list);
+    list_add_tail_crash(&data->applied_list, &applied_list);
     register_virtual_region(&data->region);
 
     return 0;


And we get:

(XEN) xsplice: xen_hello_world: Applying 1 functions
(XEN) Xen BUG at xsplice.c:963
(XEN) *** DOUBLE FAULT ***

A bit more debugging shows that "is_patch" does not iterate over
the list - (which is inline with what I expected!) - so the insertion
is safe when crashing.

Now why I get a double fault.. will have to investigate that at some
point.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel