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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.16 at 12:10, <konrad.wilk@xxxxxxxxxx> wrote:
> 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 */
> +}

This doesn't mean anything - in the absence of the BUG_ON() the
compiler is free to re-order all four assignments.

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

How does that match up with

bool_t is_patch(const void *ptr)
{
    struct payload *data;

    /*
     * No locking since this list is only ever changed during apply or revert
     * context.
     */
    list_for_each_entry ( data, &applied_list, applied_list )
    ...

i.e. where we started from here? I really think you want to at least
consider using list_add_rcu() at the insertion site, as your main
concern is that prev->next shouldn't get updated before new->next.

Jan


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