Re: [Xen-devel] [PATCH] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

On Fri, Apr 29, 2016 at 12:23:38AM +0100, Andrew Cooper wrote:
> On 28/04/2016 19:16, Konrad Rzeszutek Wilk wrote:
> > diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> > index 1b67d39..48088ce 100644
> > --- a/xen/common/xsplice.c
> > +++ b/xen/common/xsplice.c
> > @@ -1099,6 +1099,13 @@ static void xsplice_do_action(void)
> >      data->rc = rc;
> >  }
> >  
> > +static bool_t is_work_scheduled(struct payload *data)
> 
> const struct payload *data

Yes!
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

And of course 5 hours later I realized there is a more straightforward
for this. It follows the same idea but it piggyback on data->rc
being set by 'schedule_work' to -EAGAIN once work is scheduled:

It could even be rolled in "xsplice: Implement support for
applying/reverting/replacing patches."


From 83053e3f984b67dfae74cb64e75b8871b9d236ca Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Thu, 28 Apr 2016 21:22:49 -0400
Subject: [PATCH] xsplice: Check data->rc for -EAGAIN to guard against races.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently it is possible to:

1)  xc_xsplice_apply()
     \-> xsplice_action
        spin_lock(payload_lock)
             \- schedule_work()
                 data->rc=-EAGAIN
        spin_unlock(payload_lock);

2)  xc_xsplice_unload()
     \-> xsplice_action
        spin_lock(payload_lock)
             free_payload(data);
        spin_unlock(payload_lock);

.. all CPUs are quiesced.

3) check_for_xsplice_work()
     \-> apply_payload
        \-> arch_xsplice_apply_jmp
            BOOM
         data->rc =0

The reason is that state is in 'CHECKED' which changes to 'APPLIED'
once check_for_xsplice_work finishes (and it updates data->rc to zero).

But we have a race between 1) -> 3) where one can manipulate the payload
(as the state is in 'CHECKED' from which you can apply/revert and unload).

This patch adds a simple check on data->rc to see if it is in -EAGAIN
which means that schedule_work has been called for this payload.

If the payload aborts in check_for_xsplice_work (timed out, etc),
the data->rc will be -EBUSY -so one can still unload the payload or
retry the operation.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Reported-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

---
CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Tim Deegan <tim@xxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
---
---
 xen/common/xsplice.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 1b67d39..0bc7e0f 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -1363,6 +1363,9 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
*action)
         return PTR_ERR(data);
     }
 
+    if ( data->rc == -EAGAIN ) /* schedule_work has been called. */
+        goto out;
+
     switch ( action->cmd )
     {
     case XSPLICE_ACTION_UNLOAD:
@@ -1423,6 +1426,7 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
*action)
         break;
     }
 
+ out:
     spin_unlock(&payload_lock);
 
     return rc;
-- 
2.4.0


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