diff mbox

[v5,26/28] xsplice: Prevent duplicate payloads from being loaded.

Message ID 1458849640-22588-27-git-send-email-konrad.wilk@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk March 24, 2016, 8 p.m. UTC 
From: Ross Lagerwall <ross.lagerwall@citrix.com>

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
---
---
 xen/common/xsplice.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Jan Beulich April 4, 2016, 3:06 p.m. UTC | #1 
>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote:
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -566,6 +566,27 @@ static int prepare_payload(struct payload *payload,
>          if ( !payload->id.len || !payload->id.p )
>              return -EINVAL;
>      }
> +    /* Make sure it is not a duplicate. */
> +    if ( payload->id.len )

The conditional is pointless considering the one visible in context
above.

> +    {
> +        struct payload *data;
> +
> +        spin_lock_recursive(&payload_lock);
> +        list_for_each_entry ( data, &payload_list, list )
> +        {
> +            /* No way payload is on the list. */
> +            ASSERT( data != payload );
> +            if ( data->id.len &&
> +                 !memcmp(data->id.p, payload->id.p, data->id.len) )
> +            {
> +                spin_unlock_recursive(&payload_lock);
> +                dprintk(XENLOG_DEBUG, "%s%s: Already loaded as %s!\n",
> +                        XSPLICE, elf->name, data->name);
> +                return -EEXIST;
> +            }
> +        }
> +        spin_unlock_recursive(&payload_lock);

Similar question as asked elsewhere - with the lock getting dropped
here, how is the "no duplicate" state going to be ensured by the
time you actually load and insert this payload?

Jan
Konrad Rzeszutek Wilk April 4, 2016, 7:52 p.m. UTC | #2 
On Mon, Apr 04, 2016 at 09:06:47AM -0600, Jan Beulich wrote:
> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/common/xsplice.c
> > +++ b/xen/common/xsplice.c
> > @@ -566,6 +566,27 @@ static int prepare_payload(struct payload *payload,
> >          if ( !payload->id.len || !payload->id.p )
> >              return -EINVAL;
> >      }
> > +    /* Make sure it is not a duplicate. */
> > +    if ( payload->id.len )
> 
> The conditional is pointless considering the one visible in context
> above.

/me nods. Let me move it inside the braces above.
> 
> > +    {
> > +        struct payload *data;
> > +
> > +        spin_lock_recursive(&payload_lock);
> > +        list_for_each_entry ( data, &payload_list, list )
> > +        {
> > +            /* No way payload is on the list. */
> > +            ASSERT( data != payload );
> > +            if ( data->id.len &&
> > +                 !memcmp(data->id.p, payload->id.p, data->id.len) )
> > +            {
> > +                spin_unlock_recursive(&payload_lock);
> > +                dprintk(XENLOG_DEBUG, "%s%s: Already loaded as %s!\n",
> > +                        XSPLICE, elf->name, data->name);
> > +                return -EEXIST;
> > +            }
> > +        }
> > +        spin_unlock_recursive(&payload_lock);
> 
> Similar question as asked elsewhere - with the lock getting dropped
> here, how is the "no duplicate" state going to be ensured by the
> time you actually load and insert this payload?

It won't. I've removed the spinlock usage here and we are keeping the spinlock
held at xsplice_upload.
> 
> Jan
>
diff mbox

Patch

diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index d5d4b3c..a03b81a 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -566,6 +566,27 @@  static int prepare_payload(struct payload *payload,
         if ( !payload->id.len || !payload->id.p )
             return -EINVAL;
     }
+    /* Make sure it is not a duplicate. */
+    if ( payload->id.len )
+    {
+        struct payload *data;
+
+        spin_lock_recursive(&payload_lock);
+        list_for_each_entry ( data, &payload_list, list )
+        {
+            /* No way payload is on the list. */
+            ASSERT( data != payload );
+            if ( data->id.len &&
+                 !memcmp(data->id.p, payload->id.p, data->id.len) )
+            {
+                spin_unlock_recursive(&payload_lock);
+                dprintk(XENLOG_DEBUG, "%s%s: Already loaded as %s!\n",
+                        XSPLICE, elf->name, data->name);
+                return -EEXIST;
+            }
+        }
+        spin_unlock_recursive(&payload_lock);
+    }
 
     sec = xsplice_elf_sec_by_name(elf, ".xsplice.depends");
     {