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

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: keir@xen.org, ross.lagerwall@citrix.com,
	andrew.cooper3@citrix.com, mpohlack@amazon.com,
	sasha.levin@oracle.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v8.1 17/27] xsplice: Add support for bug frames.
Date: Fri, 22 Apr 2016 06:54:05 -0400	[thread overview]
Message-ID: <20160422105404.GE16225@localhost.localdomain> (raw)
In-Reply-To: <571A18FA02000078000E4A4B@prv-mh.provo.novell.com>

On Fri, Apr 22, 2016 at 04:28:42AM -0600, Jan Beulich wrote:
> >>> On 22.04.16 at 12:10, <konrad.wilk@oracle.com> wrote:
> > On Thu, Apr 21, 2016 at 12:49:09AM -0600, Jan Beulich wrote:
> >> >>> On 21.04.16 at 02:29, <konrad.wilk@oracle.com> wrote:
> >> > On Tue, Apr 19, 2016 at 02:17:35PM -0600, Jan Beulich wrote:
> >> >> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 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.

/me nods.
> 
> > 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

The thing that makes it "safe" (in lieu of your comment about
re-ordering) is that the trap code does:

1289     /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
1290     filename = bug_ptr(bug);
1291     if ( !is_kernel(filename) && !is_patch(filename) )
1292         goto die;

Which means we first try 'is_kernel'. Since the list addition cannot be
part of the payload it will always be part of the hypervisor.

> consider using list_add_rcu() at the insertion site, as your main
> concern is that prev->next shouldn't get updated before new->next.

/me nods. Let me look at that.
> 
> Jan
> 

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